Skip to content

MAINT/ENH: datetime: remove calls to PyUnicode_AsASCIIString, allow μs as an alias for us#17176

Merged
mattip merged 3 commits intonumpy:masterfrom
eric-wieser:tidy-datetime-metadata
Aug 30, 2020
Merged

MAINT/ENH: datetime: remove calls to PyUnicode_AsASCIIString, allow μs as an alias for us#17176
mattip merged 3 commits intonumpy:masterfrom
eric-wieser:tidy-datetime-metadata

Conversation

@eric-wieser
Copy link
Member

Motivated by #15317.

Happy to revert the last commit, added it primarily as a test-case

Rather than raising UnicodeEncodeError, this just raises ValueError.

Passing weird byte strings results in UnicodeDecodeError instead.
In future, we may just want to deprecate passing `bytes` objects.
This is motivated primarily as a proof that the datetime API is now unicode-safe, and generally seems harmless.
@eric-wieser eric-wieser force-pushed the tidy-datetime-metadata branch from e359477 to d3e4792 Compare August 28, 2020 12:55
@eric-wieser
Copy link
Member Author

PyPy failure is matmul again and unrelated.

def test_datetime_dtype_creation(self):
for unit in ['Y', 'M', 'W', 'D',
'h', 'm', 's', 'ms', 'us',
'μs', # alias for us
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This assumes the file encoding is utf-8, which is the default. It might help some editors to explicitly add a
# -*- coding: utf-8 -*- directive to the first line of the file.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, all non-dead versions of python assume source files are utf-8 too, so arguably an up to date editor should be able to guess the correct encoding.

If you're worried, I can also use \N{Greek small letter mu} instead.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no, it was just a nit anyway.

@mattip
Copy link
Member

mattip commented Aug 30, 2020

Looks good, thanks. This flips the default case from bytes to str (unicode), so I hope the general usage pattern for specifying datetime units is str, but anyway the performance hit should be minimal.

@mattip
Copy link
Member

mattip commented Aug 30, 2020

That PyPy failure is on windows 32-bit, which we do not test for with CPython. I don't think it is connected to gh-16744. Has it appeared often?

@mattip mattip merged commit bbe2cca into numpy:master Aug 30, 2020
@mattip
Copy link
Member

mattip commented Aug 30, 2020

Thanks @eric-wieser

@eric-wieser
Copy link
Member Author

I hope the general usage pattern for specifying datetime units is str

I'd be very surprised if it wasn't

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants