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

Improve docs for RUF012 #15804

Closed
RayBB opened this issue Jan 29, 2025 · 5 comments · Fixed by #15982
Closed

Improve docs for RUF012 #15804

RayBB opened this issue Jan 29, 2025 · 5 comments · Fixed by #15982
Labels
documentation Improvements or additions to documentation

Comments

@RayBB
Copy link
Contributor

RayBB commented Jan 29, 2025

Description

Hi there! I've been working on enabling RUF012 (Mutable class variables should not have default values) for OpenLibrary, and I believe the documentation for this rule could be enhanced to make it more actionable and clear. Specifically, I'd like to propose the following improvements:

1. Provide a Suggestion/Example for Immutable Types That Work for dict:

The current documentation for RUF012 highlights the issue of mutable default values in class variables but doesn't provide a concrete example of how to handle dict types immutably. A common solution is to use types.MappingProxyType, which creates an immutable wrapper around a dictionary. Adding an example of this would make the rule more practical and easier to implement.

Example:

from types import MappingProxyType

class MyClass:
    # Instead of a mutable dict
    MY_DICT: dict[str, str] = {}

    # Use an immutable MappingProxyType
    MY_DICT: MappingProxyType[str, str] = MappingProxyType({"key": "value"})

2. Provide an Example of Moving a Variable to Be an Instance Variable:

Another way to address mutable class variables is to move them to instance variables initialized in __init__.

Example:

class MyClass:
    # Instead of a mutable class variable
    MY_LIST: list[int] = []

    # Move it to an instance variable
    def __init__(self):
        self.my_list: list[int] = []

These additions would make the documentation more comprehensive and actionable, especially for developers who are new to the rule or Python's best practices for immutability.

I hope this is helpful. Unfortunately, I don't have time now to update the docs myself but wanted to at least create an issue for it. Thanks for your time and consideration!

@dylwil3 dylwil3 added the documentation Improvements or additions to documentation label Jan 29, 2025
@MichaReiser
Copy link
Member

Thanks for the detailed write up. Would you be interested in creating a PR to improve the documentation?

@RayBB
Copy link
Contributor Author

RayBB commented Feb 2, 2025

Sure I can make a PR. Do you wanna give any feedback on the examples now?

@MichaReiser
Copy link
Member

it looks good to me. I'd probably simplify 2 to have the annotation on the instance attribute

class A:
    mutable_default: list[int]
    immutable_default: list[int]

    def __init__(self):
        self.mutable_default = []
        self.immutable_default = []

@AlexWaygood do you have any inputs to the documentation?

@AlexWaygood
Copy link
Member

Improving the examples makes sense to me! These seem like great suggestions overall.

I'm undecided on whether it makes sense to mention MappingProxyType specifically. It's true that this is one way to fix the issue:

+ from types import MappingProxyType
+ from collections.abc import Mapping
+
  class A:
-     instance_dict: dict[str, str] = {"key": "value"}
+     instance_dict: Mapping[str, str] = MappingProxyType({"key": "value"})

But I think the more common way to fix the issue (assuming you want to keep it as an instance variable rather than a class variable) would be to do this:

  class A:
-     instance_dict: dict[str, str] = {"key": "value"}
+
+     def __init__(self) -> None:
+         self.instance_dict: dict[str, str] = {"key": "value"}

or this:

  class A:
-     instance_dict: dict[str, str] = {"key": "value"}
+     instance_dict: dict[str, str] | None = None

We could maybe mention MappingProxyType towards the bottom of the docs, but there can be costs to making the docs for a rule too long (it becomes overwhelming for users)

@RayBB
Copy link
Contributor Author

RayBB commented Feb 5, 2025

Hey folks, thanks for the input here.
I opened a PR based on your comments.
I'm not particularly attached to it so feel free to edit it directly and merge as you see fit.

#15982

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants