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

openpyxl: Class attributes set explicitly as another class' #10549

Merged

Conversation

Avasam
Copy link
Collaborator

@Avasam Avasam commented Aug 9, 2023

Has overlap with #9511, so should reduce changes there.

I searched the openpyxl source code for instances of a class attribute being explicitly set as another class'.

In most cases, those will be descriptors, which pyright will have issues with re-assigning. Luckily, they'll also mostly refer to parent definitions. For typing purposes, this can be simplified to just not including them. I still left them in as comments to make it a bit more explicit.

Similarly, mypy has issues with re-assigning properties (see python/mypy#13975 / python/mypy#6700) so I explicitly redefined those when they came from a non-parent class.

@github-actions
Copy link
Contributor

github-actions bot commented Aug 9, 2023

According to mypy_primer, this change has no effect on the checked open source code. 🤖🎉

@Avasam Avasam changed the title openpyxl: Class variables set explicitly as another class' openpyxl: Class attributes set explicitly as another class' Aug 12, 2023
Copy link
Collaborator

@srittau srittau left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks reasonable, even if I didn't trace through some of the very dynamic code.

@srittau srittau merged commit 54193d5 into python:main Aug 15, 2023
49 checks passed
@Avasam Avasam deleted the openpyxl-Class-properties-set-explicitely branch February 29, 2024 00:34
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