[WiP] FIX/ENH: include (offset, on-off seq) as a valid pattern to the linestyle validator (fix #9792)#9797
Conversation
jklymak
left a comment
jklymak
left a comment
There was a problem hiding this comment.
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.
With len(ls) == 2 you implicitly support other than tuple 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.
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.
In general we try to as forgiving as possible on inputs, I do not see a reason to be strict here.
There was a problem hiding this comment.
@tacaswell Well then should we change the docstrings that explicitly state "dash tuple" 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.
offset is overwritten here. Is not this line should be before if? (and you can replace ls[0] with offset in multiple places)
There was a problem hiding this comment.
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.
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.
That is right. I'll see how to clean this when implementing @Kojoley's below suggestion too.
afvincent
changed the title
|
Superseded by #15827; I stole some tests from this, thanks for the PR :) |
6 participants
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?