Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork7.9k
[WiP] FIX/ENH: include (offset, on-off seq) as a valid pattern to the linestyle validator (fix #9792)#9797
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
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.
These changes and tests look good to me, modulo not knowing what other edge cases might be missing...
| offset = None | ||
| # Look first for the case (offset, on-off seq). Take also care of the | ||
| # historically supported corner-case of an on-off seq like ["1", "2"]. | ||
| if (len(ls) == 2 and iterable(ls[1]) and |
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.
Withlen(ls) == 2 you implicitly support other thantuple containers (e.g.[None, [1.23, 456]]), if it is an expected behavior there is not test for it.
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.
Well it is True that I did not realized how explicit the docstring was about “dash tuple” for(offset, on-off seq.). 🐑.
I guess that it means that the validator is actually a bit too loose and should be changed to accept only tuples for the case(offset, on-off seq.)... Or do we prefer to support non-tuple cases 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.
In general we try to as forgiving as possible on inputs, I do not see a reason to be strict here.
afvincentNov 21, 2017 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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.
@tacaswell Well then should we change the docstrings that explicitly state "dashtuple" for custom line style argument (more or less everywhere :/)? Or is the currently implicit behavior fine? If we want to keep the valid pattern loose, I will at least add a few more tests with other than tuple containers, as@Kojoley suggested.
| raise ValueError( | ||
| "a linestyle offset should be None or " | ||
| "a numerical value, not {!r}.".format(ls[0])) | ||
| offset, ls = ls |
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.
offset is overwritten here. Is not this line should be beforeif? (and you can replacels[0] withoffset in multiple places)
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.
You're right... And indeed it seems that one can also factor a bit. I'll correct this for the next iteration.
| # the offset and continue with the on-off sequence candidate. | ||
| if ls[0] is not None: | ||
| try: | ||
| offset = float(ls[0]) |
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.
Do you need this assignment? I think this is just testing it can be cast to a float?
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.
That is right. I'll see how to clean this when implementing@Kojoley's below suggestion too.
Superseded by#15827; I stole some tests from this, thanks for the PR :) |
PR Summary
Currently, the linestyle validation considers the pattern
(offset, on-off sequence)as invalid, which can raise issues as linestyle can be converted internally to such a pattern. See for example the issue#9792 for an example where it occurs when existing a style context manager.To be honest, I do not remember exactly why I chose not to support this kind of linestyle patterns in
_validate_linestylewhen I made it stricter in#8040 🐑...This PR:
_validate_linestyleto accept inputs of the form(offset, (valid...) on-off sequence);test_rcparams, notably because such inputs were formerly raising exceptions;["1.23", "4.56"], which was OK before, as unfortunate it may be).With this PR, the example given in#7972 now goes smoothly and as expected.Should I add an (non image) test in
test_styleinspired from this issue, or are the updates that I made intest_rcparamsenough?PR Checklist
Should I add an entry in
api_changes?