-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
Mark artist property aliases explicitly #22749
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
base: main
Are you sure you want to change the base?
Conversation
result = ds.startswith('Alias for ') | ||
alt_result = (o._artist_property.is_alias | ||
if hasattr(o, '_artist_property') | ||
else False) | ||
assert result == alt_result | ||
return result |
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.
This code segment is temporary. I've written it to easily verify that the way of obtaining is_alias via the property is equivalent to getting that information from the docstring. Note that this is already run at import time on all artists via ArtistInspector.__init__
-> ArtistInspector.get_aliases()
because we inspect all artists for kwdocs. So if the assert holds in the tests the equivalence is proven.
I propose the following review pattern:
After two people have seen that the tests pass and have approved the PR, I'll change this whole function to only using the new way:
def def is_alias(self, o):
return (o._artist_property.is_alias
if hasattr(o, '_artist_property')
else False)
PLEASE REVIEW, BUT DO NOT MERGE after the second approval.
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.
Would it make sense to add that "Alias for..." part of the doc-string based on this? So not only provide an is_alias
boolean, but rather a string for which artist property it is an alias for?
54a73e7
to
3b600e5
Compare
Can you clarify what you're trying to achieve here? (Perhaps I just missed a background discussion...) Yes, it's a bit inelegant that ArtistInspector currently checks the docstring for "Alias for", but that could reasonably easily be fixed as all but one aliases are defined via
|
This has to be viewed in a larger context: We should make artist properties and all their aspects explicit instead of relying on heuristics like "docstring starts with " or "get_* methods with one argument". What info exactly will be needed is to be seen, but we want to at least: 1) store the simple fact that it's an artist property. 2) An optional short description for Declaration: For declaration, I find it most convenient to add decorators to the methods.
A separate declaration runs the risk of getting out of sync and is less clear in the code. Storage: I've here chosen to store the the information directly on the methods. It seems practical and good enough to me, but I haven't thought this through completely. We could alternatively have some sort of I think the main challenge is to add all decorators while making sure that they have the same as effect the other mechanisms like the heuristics and alias map before. We'll have a transition phase (maybe even only within a PR) in which both mechanisms are present and we can compare them. - The alternative would be to dump that information e.g. in a file and test against that before and after the changes. The second part of the challnage is that we need a lot of decorators, so I started only with the info 3a) (whether the property is an alias) to make sure the basic infrastructure works. We can then add more and more info in subsequent PRs. For itself, this PR may seem incomplete, but you should see it as a reviewable step in the right direction. I could do all the planned changes in a single PR, but I'm afraid that'd be completely not reviewable because I add several hundred decorators with at least information on 1), 2), 3a), 3b) and at the same time remove all the logic we currently use for artist properties. Maybe we can get rid of Whether |
It is actually not so clear to me why this is necessary. I am absolutely not opposed to doing so, but what would be the benefits?
I guess one way to decide this is how this info should propagate to subclasses, e.g. if a subclass overrides a method it will (by default) lose the attribute info -- do we want that? OTOH if it's stored in a dict(-like) in the class then the subclass should probably copy it before updating it with its own info.
I certainly understand that changing the hundreds of properties we have would not be a good idea before deciding on a final design; I'm just trying to understand what the desired endpoint is. |
Clarity and simplicity. One could discuss whether one needs the concept at all, but we're using this heavily e.g. in What I don't like about the current solution:
I'd like to get rid of ArtistInspector completely finally. I think most of the above can be addressed using a declarative approach with a decorator. |
Note that we could relatively easily rename "artist properties" to something else -- the name "property" hardly appears in the API, except as initials in
Sure, but that's something that can be done independently of the rest ("it's a name such that there is a
We could move most of the implementation of ArtistInspector to a private class and make
This was almost certainly done intentionally, given that before I introduced
Sure, that could be renamed. If AI gets deprecated as a public class (see below), the name doesn't really matter anyways though.
In practice it would seem simpler to have a decorator to exclude such cases (perhaps less pure, but avoids having to add hundreds and hundreds of decorators...)
As for "Alias for", "ACCEPTS:" was here waaaay before parameter docstring scraping (I wrote that scraping and removed ACCEPTS strings which were present in nearly all setters before that). I do agree that this specific case may be better served by a decorator, though.
No strong opinion here, but do you want to maintain a user-facing API similar to ArtistInspector? If so it may be not worth breaking the backcompat. So I guess my suggestion would be: keep the implicit "setters with one arg are properties" (perhaps with |
Yes, it's mostly in docs. If you have a good name, that would certainly be a consideration to reduce confusion with python properties.
I'm afraid we'll have to add decorators in many cases anyway to get better docs for kwargs, c.f. #22699
Not yet decided. IMHO users should not need this and less public API is less maintenance burden. If it stays, I'd rather have it only look up data that has previously been set/declared. I don't like the complex logic it does.
Let's take it step by step. I'll put this on hold, and reactivate #22699 for "a decorator that replaces ACCEPTS" everything else can come later and we can then decide how to approach the individual parts. |
Probably a better question for the native speakers :) I can't think of anything right now. I guess after #22699 goes in first, we can always decide whether to also decorate everything else or manually mark exclusions. So that sounds good to me. |
"Artist setting"? Since it's something you set? |
PR Summary
This is a first step for making artist properties explicit.
Overall goal
We call the variables associated with getters and setters of Artists artist properties. e.g.
linestyle
is an artist property ofLine2D
. Artist properties can have aliases and are often used as keyword arguments to artist factory functions.Issue: We don't don't declare artist properties explicitly, but instead try to infer them based on heuristic rules, mainly in
ArtistInspector
. This is cumbersome and fragile because people might not be aware of these rules when they add new properties or change existing ones.Solution: The setters and getters assoicated with artist properties should be explicitly marked as such by adding an attribute
_artist_property
to these methods, usually via a decorator.This PR
_artist_property
attribute, and the underlying datastructureArtistPropertyInfo
. The datastructure already contains kwdoc (c.f. Proof of concept for adding kwdoc content to properties using a decorator #22699) and is_alias.ArtistInsprector.is_alias()
Later PRs
will replace more and more of the ArtistInspector logic, such as
get_setters()
,get_valid_values()
/ kwdocs;properties()
. But for that we have to massively introduce the decorator and therefore it's better to do it step by step.