-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
Improve error for invalid format strings / misspelled data keys. #23088
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
Conversation
bd952fb
to
d7bf480
Compare
@@ -118,7 +118,7 @@ def __call__(self, ax, renderer): | |||
self._transform - ax.figure.transSubfigure) | |||
|
|||
|
|||
def _process_plot_format(fmt): | |||
def _process_plot_format(fmt, *, ambiguous_fmt_datakey=False): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bonus points if you copy the docstring for the parameter here as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The docstring was already not numpydoc'ed (and doing properly so is slightly messy due to, well, the messy API), so I'll skip these points for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor wording suggestion to avoid the double negative, but it looks good to me.
@@ -163,31 +163,31 @@ def _process_plot_format(fmt): | |||
except ValueError: | |||
pass # No, not just a color. | |||
|
|||
errfmt = ("{!r} is neither a data key nor a valid format string ({})" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
errfmt = ("{!r} is neither a data key nor a valid format string ({})" | |
errfmt = ("{!r} is not a data key or a valid format string ({})" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a bit of an Oxford comma thing, but I would tend to say neither...nor here. It's correct and a bit more formal.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do prefer neither/nor, so I'll go back to that :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good. :) I'm not formal enough in my speaking it seems!
b40e6dd
to
d7bf480
Compare
I'll take the liberty of merging because I don't think @greglucas was that worried about the grammar change ;-) |
PR Summary
Closes #23083 (IMO).
FWIW I agree that the current behavior can be a bit confusing (I'm not sure really like the whole data kwarg business due to the confusion it creates in combination with plot()'s weird signatures, but that ship has sailed long ago).
PR Checklist
Tests and Styling
pytest
passes).flake8-docstrings
and runflake8 --docstring-convention=all
).Documentation
doc/users/next_whats_new/
(follow instructions in README.rst there).doc/api/next_api_changes/
(follow instructions in README.rst there).