Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ENH: improve error messages when add_field receives a malformed function argument #3921

Merged
merged 5 commits into from
Jun 6, 2022

Conversation

neutrinoceros
Copy link
Member

@neutrinoceros neutrinoceros commented May 11, 2022

PR Summary

This makes life easier for clumsy users like myself who can never remember what argument goes first in a derived field function, so you get a clear error message instead of whatever happens when the malformed function gets called with reversed arguments. Incidentally adds support and tests for passing arbitrary callables as the function argument.

edit: I also reworked the way units="auto" works to make the dimensions argument optional even in that case. This is an opinionated change but I think it's reasonable as long as a warning is displayed when needed.

@neutrinoceros neutrinoceros added api-consistency naming conventions, code deduplication, informative error messages, code smells... enhancement Making something better labels May 11, 2022
@neutrinoceros neutrinoceros marked this pull request as draft May 11, 2022 11:53
@neutrinoceros neutrinoceros force-pushed the enh_errors_add_field branch 5 times, most recently from 2f63eaf to c08c55d Compare May 12, 2022 06:32
@neutrinoceros neutrinoceros added this to the 4.1.0 milestone May 12, 2022
@neutrinoceros neutrinoceros changed the title ENH: improve error messages when add_field received a malformed function argument ENH: improve error messages when add_field receives a malformed function argument May 12, 2022
@neutrinoceros neutrinoceros marked this pull request as ready for review May 12, 2022 07:28
@neutrinoceros neutrinoceros marked this pull request as draft May 13, 2022 13:02
@neutrinoceros neutrinoceros marked this pull request as draft May 13, 2022 13:02
@neutrinoceros
Copy link
Member Author

switched to draft because I realised that in my tests, I was adding fields that are inaccessible since I provided neither dimensions or units. I feel like the add_field method should raise an error instead of positioning it to actual data access time, but this will probably require some minor changes in the API, so I need to think about it.

@neutrinoceros
Copy link
Member Author

Failures are unrelated and likely to be caused by pypa/pip#11116

@neutrinoceros neutrinoceros force-pushed the enh_errors_add_field branch 3 times, most recently from 2c73acd to 186ef7e Compare May 16, 2022 18:15
@matthewturk
Copy link
Member

edit: I also reworked the way units="auto" works to make the dimensions argument optional even in that case. This is an opinionated change but I think it's reasonable as long as a warning is displayed when needed.

I have found the units="auto" functionality to be confusing; my reading of this was always that it would identify whatever units came out, and allow those. It wouldn't precisely allow for silent failures, but it would stop validating the units, and instead be descriptive rather than prescriptive. Is that the new behavior?

@neutrinoceros
Copy link
Member Author

Kinda. I'm not sure what you mean by "descriptive rather than prescriptive", and I'm not sure that I can say that it doesn't allow for silent failures: with the new behaviour, validation can be omitted completely (but you do get a warning)

@matthewturk
Copy link
Member

I guess what I mean is, validation will fail, but you still get the units that come out of the field. i.e., it still tracks, but does not do validation against a priori values

@neutrinoceros
Copy link
Member Author

Yes I think that's correct.

@neutrinoceros neutrinoceros marked this pull request as ready for review May 17, 2022 16:01
@neutrinoceros neutrinoceros added UX user-experience labels May 19, 2022
Copy link
Member

@matthewturk matthewturk left a comment

Choose a reason for hiding this comment

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

I'm generally OK with this. I have a question though: without passing judgment on them, can you estimate how many people might have their derived fields broken by this? 0, a few, or lots?

@neutrinoceros
Copy link
Member Author

I would naively assume none to a few, but we may want to be slightly more cautious and raise a warning instead of an error for a while (and explicitly state that it'll be an error in the future). Your call :)

@matthewturk
Copy link
Member

I don't think that is necessary.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-consistency naming conventions, code deduplication, informative error messages, code smells... enhancement Making something better UX user-experience
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants