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 2 #10286

Open
wants to merge 28 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
This pull request addresses Part of RUF012 Rule - Task 2, with a focus on enhancing code quality and resolving linting issues related to mutable class attributes.

Technical Details:

  • Updated Files: The ClassVar annotations were added to mutable class attributes in two files, ensuring they are treated as class-level attributes. This change resolves the RUF012 linting issues.
  • Remaining File: The update could not be applied to plugins/worksearch/schemes/editions.py due to a shared base class with the files mentioned in Task 1, which would introduce an overriding issue. Therefore, the change was not feasible for this file.
  • Type Safety: Added proper type hints for better clarity and type safety throughout the affected files.

Testing:
Ran ruff check --select RUF012 to confirm that no further warnings related to mutable class attributes remain.

Screenshot:
N/A
Stakeholders:
@RayBB

@ananyakaligal ananyakaligal changed the title Task 2 (RUF012) Fixed mutable class Defaults- Task 2 Jan 6, 2025
@RayBB RayBB added Affects: Developers Needs: Staff / Internal Reviewed a PR but don't have merge powers? Use this. labels Jan 7, 2025
Copy link
Collaborator

@RayBB RayBB left a comment

Choose a reason for hiding this comment

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

As far as I'm concerned this looks good.
However, I'm going to need to get a second opinion from staff

@RayBB RayBB added Needs: Submitter Input Waiting on input from the creator of the issue/pr [managed] and removed Needs: Staff / Internal Reviewed a PR but don't have merge powers? Use this. labels Jan 15, 2025
@RayBB
Copy link
Collaborator

RayBB commented Jan 16, 2025

@ananyakaligal I looked at this with Drini and we realized we probably don't want all these values to be mutable.
Can you review and add non-mutable options to this (unless it needs to be mutable).

See https://docs.astral.sh/ruff/rules/mutable-class-default/

@ananyakaligal
Copy link
Contributor Author

@RayBB sure, I will work on this.

@RayBB
Copy link
Collaborator

RayBB commented Jan 21, 2025

@ananyakaligal any update on this?

@ananyakaligal
Copy link
Contributor Author

Hey @RayBB, My schedule has been pretty packed, and I'm working on the preview button right now. The CSS part of that is a bit much and very specific, so it's taking some time. I'll definitely work on this by the end of this week. My apologies!

@RayBB RayBB marked this pull request as draft January 21, 2025 17:42
@RayBB
Copy link
Collaborator

RayBB commented Jan 21, 2025

No problem. Marking as draft until it's ready 👍

@github-actions github-actions bot removed the Needs: Submitter Input Waiting on input from the creator of the issue/pr [managed] label Jan 23, 2025
@ananyakaligal
Copy link
Contributor Author

working on it

@ananyakaligal ananyakaligal marked this pull request as ready for review January 23, 2025 22:00
@ananyakaligal
Copy link
Contributor Author

ananyakaligal commented Jan 23, 2025

@RayBB I used MappingProxyType to ensure immutability, as I encountered dictionaries throughout and wanted to maintain consistency. Now, I will investigate the files where the class variable overriding issue popped up.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Someone else is working on it
Development

Successfully merging this pull request may close these issues.

2 participants