Skip to content

BUG: Unify handling of string enum converters#16008

Merged
seberg merged 1 commit intonumpy:masterfrom
eric-wieser:tidy-converter-errors
Apr 27, 2020
Merged

BUG: Unify handling of string enum converters#16008
seberg merged 1 commit intonumpy:masterfrom
eric-wieser:tidy-converter-errors

Conversation

@eric-wieser
Copy link
Copy Markdown
Member

@eric-wieser eric-wieser commented Apr 17, 2020

Builds upon gh-16007, marking as draft until that's resolved

This fixes a collection of bugs:

Comment on lines 2761 to 2765
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This test was actually calling x.flatten(dict(order=...)), which fails no matter what ... is.

@eric-wieser

This comment has been minimized.

@eric-wieser eric-wieser changed the base branch from master to maintenance/1.18.x April 22, 2020 07:07
@eric-wieser eric-wieser changed the base branch from maintenance/1.18.x to master April 22, 2020 07:07
@eric-wieser eric-wieser marked this pull request as ready for review April 22, 2020 07:07
@eric-wieser
Copy link
Copy Markdown
Member Author

CI failure is unrelated

@eric-wieser eric-wieser requested a review from seberg April 24, 2020 08:50
Copy link
Copy Markdown
Member

@seberg seberg left a comment

Choose a reason for hiding this comment

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

LGTM, not much to discuss about, just some thoughts really. Could also let all the functions return -1, since that is unreachable code right now, that ensures an incorrect error rather than incorrect result if someone edits a converter incorrectly.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why the length == 2 its not like it micro-optimizes even, since that would be the error path?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Because otherwise we allow trailing nulls.

@eric-wieser
Copy link
Copy Markdown
Member Author

Could also let all the functions return -1, since that is unreachable code right now, that ensures an incorrect error rather than incorrect result if someone edits a converter incorrectly.

Don't know what you mean by that.

This fixes a collection of bugs:

* Confusion between `TypeError` and `ValueError`
* Allowing invalid arguments if they are valid up to the first embedded null
* Producing better errors for non-ascii unicode strings
@eric-wieser eric-wieser force-pushed the tidy-converter-errors branch from 8c67f5e to 35b0a05 Compare April 27, 2020 11:13
@eric-wieser eric-wieser requested a review from seberg April 27, 2020 12:11
@seberg
Copy link
Copy Markdown
Member

seberg commented Apr 27, 2020

Thanks for the patience. The diff looks so unwieldy ;), but the code is much much nicer now. Thanks!

@seberg seberg merged commit cc7038f into numpy:master Apr 27, 2020
eric-wieser added a commit to eric-wieser/numpy that referenced this pull request Apr 30, 2020
This uses the helper function added from numpygh-16008, which is renamed and documented to justify its use in a second module.

This does not implement the length-checking needed to reject trailing nulls.
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.

4 participants