Skip to content
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

Respect ClassVar variable annotation #150

Merged
merged 2 commits into from
Apr 27, 2023

Conversation

viccie30
Copy link
Contributor

According to PEP 526 annotated attributes in a class body are instance variables by default, unless explicitly overridden by using typing.ClassVar.

This PR makes 2 changes:

  1. Honor typing.ClassVar in class variable annotations.
  2. Merge the annotations in the class body and in __init__().

@viccie30 viccie30 force-pushed the fix-class-attribute-annotations branch from 5dc444a to 7e638e7 Compare April 17, 2023 21:36
@pawamoy
Copy link
Member

pawamoy commented Apr 23, 2023

Hey @viccie30, thanks for the PR!

I think we need to distinguish three cases:

  1. attribute is annotated with ClassVar: set class-attribute label only. If attribute is redfined in __init__, don't bother checking: that's a user error, keep the current flow (add instance-attribute label, etc.)
  2. attribute is only annotated: since the attribute is not accessible through the class at runtime, but only through instances, set instance-attribute label
  3. attribute is annotated and assigned: set class-attribute label, and keep the current flow (add instance-attribute label if defined again in __init__ etc.)

I think we previously handled case 3, and your PR adds support for case 1. We now just need to support case 2 🙂 WDYT? We can just check if the AnnAssign node has a value or not to decide whether to set the class-attribute or instance-attribute label.

@viccie30
Copy link
Contributor Author

Thanks for looking into the PR.

I agree that there's 3 cases. Case 1 is user error, like you say. Case 2 is already supported in this PR, through line 566 in src/griffe/agents/visitor.py.

I'm not sure what you mean with case 3. According to the PEP if a non-ClassVar annotated attribute at class scope gets a value assigned at class scope, it's still an instance variable and not a class variable. This PR handles it as such. I have added new test cases to check this.

@viccie30 viccie30 force-pushed the fix-class-attribute-annotations branch from 7e638e7 to 2111759 Compare April 25, 2023 08:20
@pawamoy
Copy link
Member

pawamoy commented Apr 25, 2023

Thanks. Well to me z should also have the class-attribute label, because it is indeed accessible from the class at runtime. I'm more concerned about what users of the code can/will do rather than what the PEP says.

According to [PEP 526](https://peps.python.org/pep-0526/#class-and-instance-variable-annotations)
annotated attributes in a class body are instance variables by default,
unless explicitly overridden by using
[`typing.ClassVar`](https://docs.python.org/3/library/typing.html#typing.ClassVar).

Griffe also considers annotated attributes with a default value
class variables.
@viccie30 viccie30 force-pushed the fix-class-attribute-annotations branch from 2111759 to 688e7b0 Compare April 26, 2023 06:04
@viccie30
Copy link
Contributor Author

I have added a branch in handle_attribute() to mark initialized annotated variables as class variables.

@pawamoy
Copy link
Member

pawamoy commented Apr 27, 2023

I've added the instance-attribute label when the attribute has a value but is not annotated as ClassVar. LGTM now, merging, thanks!

@pawamoy pawamoy merged commit 60e01c1 into mkdocstrings:master Apr 27, 2023
@viccie30 viccie30 deleted the fix-class-attribute-annotations branch May 23, 2023 15:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants