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 ArtistInspector.get_aliases.#12037
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
The docstring parsing in get_aliases was brittle and didn't handle thefinal dot that cbook._define_aliases added to the docstring, so as ofmaster it would return the keys with an additional dot. Instead, use aregex which will loudly error out if the format is incorrect.Also replace {key: None, ...} mapping simulating a set to, well, a set.(The use of sets instead of lists was added ined9c2b5 apparently tohandle the case where the same alias is added twice.)(Yes, the whole alias machinery could now be simplified e.g. by addingan `is_mpl_alias` attribute on the aliases instead.)
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.
Seems fine to me. I guess get_aliases is almost internal API that we don't need to worry about the API change too much?
If you consider get_aliases to be public API, then the exact format of the alias docstrings also becomes public API, which sounds... awkward. |
…037-on-v3.0.xBackport PR#12037 on branch v3.0.x (Fix ArtistInspector.get_aliases.)
The docstring parsing in get_aliases was brittle and didn't handle the
final dot that cbook._define_aliases added to the docstring, so as of
master it would return the keys with an additional dot. Instead, use a
regex which will loudly error out if the format is incorrect.
Also replace {key: None, ...} mapping simulating a set to, well, a set.
(The use of sets instead of lists was added ined9c2b5 apparently to
handle the case where the same alias is added twice.)
(Yes, the whole alias machinery could now be simplified e.g. by adding
an
is_mpl_alias
attribute on the aliases instead.)At least the parsing fix should be considered RC as the issue came in in
#9475.
PR Summary
PR Checklist