Skip to content

[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.

Sign up for GitHub

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

Closed
wants to merge 2 commits into from
Closed

[WiP] FIX/ENH: include (offset, on-off seq) as a valid pattern to the linestyle validator (fix #9792) #9797

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
612d6d0
add offset-onoffseq pattern to ls validation
afvincent Nov 16, 2017
ffd3434
update the relevant test
afvincent Nov 16, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
add offset-onoffseq pattern to ls validation
  • Loading branch information
@afvincent
afvincent committed Nov 16, 2017
commit 612d6d0c27cff76bbcda3cf4bdcd4844f10ed95f
22 changes: 19 additions & 3 deletions lib/matplotlib/rcsetup.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@
import warnings
import re

from matplotlib.cbook import mplDeprecation, deprecated, ls_mapper
from matplotlib.cbook import mplDeprecation, deprecated, ls_mapper, iterable
from matplotlib.fontconfig_pattern import parse_fontconfig_pattern
from matplotlib.colors import is_color_like

Expand Down Expand Up @@ -931,13 +931,29 @@ def _validate_linestyle(ls):
"sequence nor a valid string.".format(ls))

# Look for an on-off ink sequence (in points) *of even length*.
# Offset is set to None.
# Offset is set to None if not present.
try:
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
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

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?

Copy link
Member

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.

Copy link
Contributor Author

@afvincent afvincent Nov 21, 2017 •
edited
Loading

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 "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.

not isinstance(ls[0], six.string_types)):
# That is likely to be a pattern (offset, on-off seq), so extract
# the offset and continue with the on-off sequence candidate.
if ls[0] is not None:
try:
offset = float(ls[0])
Copy link
Member

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?

Copy link
Contributor Author

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.

except ValueError:
raise ValueError(
"a linestyle offset should be None or "
"a numerical value, not {!r}.".format(ls[0]))
offset, ls = ls
Copy link
Member

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 before if? (and you can replace ls[0] with offset in multiple places)

Copy link
Contributor Author

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.


if len(ls) % 2 != 0:
raise ValueError("the linestyle sequence {!r} is not of even "
"length.".format(ls))

return (None, validate_nseq_float()(ls))
return (offset, validate_nseq_float()(ls))

except (ValueError, TypeError):
# TypeError can be raised inside the instance of validate_nseq_float,
Expand Down