Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork7.9k
Fix validation of linestyle in rcparams and cycler.#15827
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
2b5b24e to664ab35Comparelib/matplotlib/rcsetup.py Outdated
| except ValueError: | ||
| pass | ||
| try: | ||
| ls = ast.literal_eval(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.
Is this only there to support reading rc_files? If so, I would mention this in a comment.
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's literally the case forall functions in rcsetup.py...
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.
But most of them work implicitly because the don't parse nested structures. Thenstr -> other type is trivial.
Actually, I'd be in favor of making rcfiles YAML compliant. They mostly are already. We could delegate all the string parsing to the YAML parser. Then, the validators would only operate on the actual python types (not on their string representation). This would also be beneficial for using validators when setting rcParams directly in the code. That's for another time/discusion, but the comment would help to remove the string parsing from the validator later.
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.
Added a comment.
I am strongly opposed to using yaml here; it's the wrong tool for the task: it's not expressive enough to describe things that are actually problematic in the current rc syntax (axes.prop_cycler, path.effects), while bringing in a lot of complexity (a typical one being the many, many different kind of strings that exist) that is unwarranted.
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.
axes.prop_cycle could well be described by a dict
axes.prop_cycle: color: ['1f77b4', 'ff7f0e', '2ca02c'] linestyle: ['-', '--', ':']But no need to go into that discussion here.
Correcly use _validate_linestyle instead of validate_stringlist in thecycler linestyle validator. For compat, support both the (on, off, on,off, ...) and (offset, (on, off, on, off, ...)) forms. Don't supportpassing on, off, etc. as *individual* separate strings anymore as that'snot a realistic use case (unlike passing the whole thing as a singlestring, which is supported for parsing the rc file).
664ab35 to4899580CompareAnybody can merge after CI pass. |
Uh oh!
There was an error while loading.Please reload this page.
Correcly use _validate_linestyle instead of validate_stringlist in the
cycler linestyle validator. For compat, support both the (on, off, on,
off, ...) and (offset, (on, off, on, off, ...)) forms. Don't support
passing on, off, etc. asindividual separate strings anymore as that's
not a realistic use case (unlike passing the whole thing as a single
string, which is supported for parsing the rc file).
Closes#15825 (the corresponding test is that (0, [1, 2]) no longer fails but is instead parsed as (0, [1, 2])).
Makeshttps://github.com/matplotlib/matplotlib/pull/8040/files#diff-2423339af6767c3b30e1882f7c8d8f54R30 actually work (it didn't before).
Note that _validate_stringlist needed to be moved up in rcsetup.py to come before the _prop_validators dict.
Edit: Alsocloses#9792 and supersedes#9797, from which some test cases were taken.
PR Summary
PR Checklist