-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
ENH: Stricter validation of line style rcParams (and extended accepted types for grid.linestyle
)
#8040
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: Stricter validation of line style rcParams (and extended accepted types for grid.linestyle
)
#8040
Conversation
It does not make the validation weaker at least. |
I could make the validation stronger in the case of strings if you think it is better. |
lib/matplotlib/rcsetup.py
Outdated
pass | ||
|
||
raise ValueError("'grid.linestyle' must be a string or " + | ||
"a even-length sequence of floats.") |
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.
Note to myself for tomorrow: “an even” (done)
Shouldn't this be applied for all linestyles, not just grid? |
👍 to @dopplershift suggestion to add this to all of the rcparams that take linestyle. Interestingly, for negative contours, there is a commit from 2007 (4682b0c) which deprecated passing in that linestyle as a on/off pattern. |
I am not sure it is exactly the same situation:
By the way, I have the feeling that the deprecated validation for the negative contour line style is a bit restrictive: it seems like only 2-element sequences are accepted (i.e. [3, 1, 1, 1] would be rejected for example, if I am correct…). |
Counterpoint: why on earth, from a user standpoint, is configuring grid linestyle different from the other linestyles? I get the programmer concerns, but from a UX perspective, we're giving users a new thing to learn with no justification. Feels like a set of features that grew over time (which it is) rather than a cohesive design (which is what people want). |
Fair enough :), I am going to relax the validation of all the 'linestyle' rcParams, i.e.: In [4]: [key for key in plt.rcParams if u'linestyle' in key]
Out[4]:
[u'boxplot.boxprops.linestyle',
u'boxplot.capprops.linestyle',
u'boxplot.flierprops.linestyle',
u'boxplot.meanprops.linestyle',
u'boxplot.medianprops.linestyle',
u'boxplot.whiskerprops.linestyle',
u'contour.negative_linestyle', # beware of the existing deprecation
u'grid.linestyle',
u'lines.linestyle'] to accept on-off sequences. |
Now all the rcParams related to a line style (but How should I handle the case of |
Sorry, my current text editor does not yell at me when scripts are not PEP8-compliant… The other CI failure does not seem related (about “mathtext_dejavusans_21”). |
On 2017/02/09 9:08 AM, Adrien F. Vincent wrote:
How should I handle the case of |contour.negative_linestyle|? It seems
like a long time ago, one could pass an on-off ink sequence, but then is
was deprecated to only accept a named line style (see
|validate_negative_linestyle_legacy| in |rcsetup.py|). Should I
deprecate the deprecation (\o/) to use the new validator
|validate_linestyle| too? /If it is, what has to be done: where can I
find doc on the (current) deprecation process?/
I think all you have to do is remove the deprecation stuff and use the
new validator.
|
I removed the former validators There may be a (minor) break of behavior: in the new validator, I have chosen to be case-sensitive (to discourage users' jokes like “DaSHdOtTed”), while the former validator for |
lib/matplotlib/rcsetup.py
Outdated
|
||
# A validator for all possible line styles, the named ones *and* | ||
# the on-off ink sequences. | ||
def validate_linestyle(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.
can we make this a private function? I don't think it should be added to our public API.
lib/matplotlib/rcsetup.py
Outdated
@@ -888,6 +872,36 @@ def validate_animation_writer_path(p): | |||
modules["matplotlib.animation"].writers.set_dirty() | |||
return p | |||
|
|||
# A validator dedicated to the named line styles, based on the items in | |||
# ls_mapper, and a list of possible strings read from Line2D.set_linestyle | |||
validate_named_linestyle = ValidateInStrings('linestyle', |
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.
same here. this should not appear in our public API.
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.
Ok I'll do that! (I was hesitating because most of the other validators are public).
Speaking of public API, I think I've done something wrong (:sheep:) by genuinely getting rid off of validate_negative_linestyle
and validate_negative_linestyle_legacy
as they were both part of the public API. I should have used some deprecation warning instead, shouldn't I? Or at least just leave them (because if validate_linestyle
is made private, there will be no alternative)?
hmmm, my knee-jerk reaction is that maybe it should remain case-insensitive
with a deprecation to become case-sensitive? I don't remember, but are
colors case-sensitive? If they are case-insensitve, then maybe it makes
sense for everything to be case-insensitive.
…On Fri, Feb 10, 2017 at 4:36 AM, Adrien F. Vincent ***@***.*** > wrote:
I removed the former validatorsvalidate_negative_contour and
validate_negative_contour_legacy, to now consistently rely on the new
validator for all the line style-related rcParams.
There may be a (minor) break of behavior: in the new validator, I have
chosen to be case-sensitive (to discourage users' jokes like “DaSHdOtTed”),
while the former validator for contour.negative_linestyle was not (the
ignorecase kwarg was set to True). *Was this a mistake?*
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#8040 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AARy-JQKMBBMs12sXCxP6J8Vw7l8sKZIks5rbC-zgaJpZM4L5reS>
.
|
I'm only in favor of limiting user choice to prevent dangerous behavior/common mistakes--not silly/poor choices. Having said that, it's much more important to me for it to be consistent with the rest of the library (like @WeatherGod said). |
From what I understand, color validation is case insensitive. In ...
# Named color.
try:
# This may turn c into a non-string, so we check again below.
c = _colors_full_map[c.lower()]
... where Anyway, I guess that making |
The new validation scheme is now private and case-insensitive :). I have reintroduced the former validators related to the |
One last thing that bothers me a bit is that the (newly deprecated) former validators ---------------------------------------------------------------------------
AttributeError Traceback (most recent call last)
<ipython-input-2-667b565f2256> in <module>()
----> 1 validate_negative_linestyle_legacy([1, 2])
/home/adrien/matplotlib/lib/matplotlib/cbook/deprecation.pyc in wrapper(*args, **kwargs)
204 def wrapper(*args, **kwargs):
205 warnings.warn(message, mplDeprecation, stacklevel=2)
--> 206 return func(*args, **kwargs)
207
208 old_doc = textwrap.dedent(old_doc or '').strip('\n')
/home/adrien/matplotlib/lib/matplotlib/rcsetup.pyc in validate_negative_linestyle_legacy(s)
549 def validate_negative_linestyle_legacy(s):
550 try:
--> 551 res = validate_negative_linestyle(s)
552 return res
553 except ValueError:
/home/adrien/matplotlib/lib/matplotlib/cbook/deprecation.pyc in wrapper(*args, **kwargs)
204 def wrapper(*args, **kwargs):
205 warnings.warn(message, mplDeprecation, stacklevel=2)
--> 206 return func(*args, **kwargs)
207
208 old_doc = textwrap.dedent(old_doc or '').strip('\n')
/home/adrien/matplotlib/lib/matplotlib/rcsetup.pyc in validate_negative_linestyle(s)
540 "deprecation warning for more information."))
541 def validate_negative_linestyle(s):
--> 542 return _validate_negative_linestyle(s)
543
544
/home/adrien/matplotlib/lib/matplotlib/rcsetup.pyc in __call__(self, s)
64 def __call__(self, s):
65 if self.ignorecase:
---> 66 s = s.lower()
67 if s in self.valid:
68 return self.valid[s]
AttributeError: 'list' object has no attribute 'lower' I think it may be fixed by simply catching |
The Travis failure seems unrelated to this PR. It is complaining about |
Probably not worth it.
…On Mon, Feb 13, 2017 at 7:54 PM, Adrien F. Vincent ***@***.*** > wrote:
One last thing that bothers me a bit is that the (newly deprecated) former
validators validate_negative_linestyle and validate_negative_linestyle_
legacy do not seem bugfree (but even before this PR they were not tested,
so it seems to have stayed under the radar…). See for example this
traceback when (the current) validate_negative_linestyle_legacy is called
with a simple 2-element list:
---------------------------------------------------------------------------AttributeError Traceback (most recent call last)<ipython-input-2-667b565f2256> in <module>()----> 1 validate_negative_linestyle_legacy([1, 2])
/home/adrien/matplotlib/lib/matplotlib/cbook/deprecation.pyc in wrapper(*args, **kwargs)
204 def wrapper(*args, **kwargs):
205 warnings.warn(message, mplDeprecation, stacklevel=2)--> 206 return func(*args, **kwargs)
207
208 old_doc = textwrap.dedent(old_doc or '').strip('\n')
/home/adrien/matplotlib/lib/matplotlib/rcsetup.pyc in validate_negative_linestyle_legacy(s)
549 def validate_negative_linestyle_legacy(s):
550 try:--> 551 res = validate_negative_linestyle(s)
552 return res
553 except ValueError:
/home/adrien/matplotlib/lib/matplotlib/cbook/deprecation.pyc in wrapper(*args, **kwargs)
204 def wrapper(*args, **kwargs):
205 warnings.warn(message, mplDeprecation, stacklevel=2)--> 206 return func(*args, **kwargs)
207
208 old_doc = textwrap.dedent(old_doc or '').strip('\n')
/home/adrien/matplotlib/lib/matplotlib/rcsetup.pyc in validate_negative_linestyle(s)
540 "deprecation warning for more information."))
541 def validate_negative_linestyle(s):--> 542 return _validate_negative_linestyle(s)
543
544
/home/adrien/matplotlib/lib/matplotlib/rcsetup.pyc in __call__(self, s)
64 def __call__(self, s):
65 if self.ignorecase:---> 66 s = s.lower()
67 if s in self.valid:
68 return self.valid[s]
AttributeError: 'list' object has no attribute 'lower'
I think it may be fixed by simply catching AttributeErrorexception in the
same way as ValueError exceptions are caught in
validate_negative_linestyle_legacy. *Should I do this fix too, or is it
not worth it due to the deprecation?*
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#8040 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AARy-NIs-OGzC-_gEfX-W1zL2jDaFlY9ks5rcPstgaJpZM4L5reS>
.
|
lib/matplotlib/rcsetup.py
Outdated
|
||
|
||
@deprecated('3.0.0?', pending=True, |
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.
Why can't we deprecate this "for 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.
So, something like
@deprecated(the version number you will kindly suggest, # :)
addendum=(" See 'validate_negative_linestyle_legacy' " +
"deprecation warning for more information.")
)
would be better?
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.
Yes :D
I suggest deprecating this in 2.1, which means no backporting. Is that a problem?
grid.linestyle
grid.linestyle
I'd like to understand why the deprecations are pending, and why we are deprecating for 3.0.0 (and not for 2.3). Else it looks good (I'll approve one @afvincent clarifies this point). |
@NelleV Well… It simply reflects that I do not understand how the process of deprecation goes ^^… |
Sidenote: I think a |
Deprecated in 2.1 means it will be removed in 2.3. 👍 on the what's new entry. |
I think it is fine to push this to 2.1 (changed my mind). |
Ok, the deprecations now start at 2.1. And I have added a PS: I have changed the PR title because actually all line style rcParams are now affected by this PR ;). |
grid.linestyle
grid.linestyle
)
Thanks! |
grid.linestyle
)grid.linestyle
)
A possible “solution” to #7991 (suggested by @tacaswell on Gitter, February 7, 2017 12:09 PM) . The user could now use a sequence like [1.0, 3.0] for the rcParam
grid.linestyle
.The validation is not extensive, partly because I was not sure it should be performed here. For example, one can perfectly pass a value like u'Hello world' without the new function
validate_grid_linestyle
raising an error. Should I set the only accepted strings to be incbook.ls_mapper
andcbook.ls_mapper_r
?Edit: typo + link to Gitter.