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

(RUF012) Fixed mutable class Defaults- Task 1 #10283

Draft
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

ananyakaligal
Copy link
Contributor

@ananyakaligal ananyakaligal commented Jan 6, 2025

Part of #10196
NOTE: These are NOT automated changes, they are done by hand

Prevents accidental modifications of mutable class attributes at the instance level.
Aligns with project style guidelines enforced by Ruff.

Technical
Updated mutable class attributes in 3 files to use typing.ClassVar for clarity and correctness.
Added ClassVar annotations to mutable class attributes to ensure they are treated as class-level attributes.
Ensured proper type hints for clarity and type safety.

Testing
Ran ruff check --select RUF012 to confirm no further warnings.

Stakeholders
@RayBB

@ananyakaligal
Copy link
Contributor Author

I closed the previous draft pull request because I ran into some errors on that branch and couldn't continue working on it. I created a new branch and am still working on it.

@ananyakaligal
Copy link
Contributor Author

Hello @RayBB,
I was working on fixing RUF 012 of TASK-1 and was annotating the variables with ClassVar. However, after adding ClassVar, I encountered the following error:
Cannot override instance variable (previously declared on base class "SearchScheme") with class variable

What happened:
In the base class SearchScheme, variables like field_name_map, sorts, and default_fetched_fields are instance variables. When I added ClassVar in the derived class, it treated them as class variables, which caused a conflict. This is because instance variables cannot be overridden by class variables, leading to the error.

Clarification:
All three files assigned to me as part of TASK-1 are using the same base class SearchScheme. Since the class variables were already correctly defined in the base class, no changes are needed to these files. Everything should be fine as is. Please correct me if I am wrong, and kindly assign TASK-2 if this is correct.

@RayBB RayBB marked this pull request as draft January 7, 2025 04:28
@RayBB
Copy link
Collaborator

RayBB commented Jan 7, 2025

@ananyakaligal I need to catchup on many other of the issues before digging into this one. For now lets leave it as a draft and I'll follow up with you when I can evaluate 👍
But yes go ahead and work on the other one :)

@RayBB RayBB self-assigned this Jan 7, 2025
@ananyakaligal
Copy link
Contributor Author

@RayBB Thankyou, please do tag me after you evaluate it because the rule hasn't been enabled yet, and I can do it later.

@RayBB RayBB added Needs: Staff / Internal Reviewed a PR but don't have merge powers? Use this. and removed Needs: Staff / Internal Reviewed a PR but don't have merge powers? Use this. labels Jan 15, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Waiting on Me
Development

Successfully merging this pull request may close these issues.

2 participants