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.

Sign up for GitHub

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

Jump to bottom

Look into ruff rule RUF012 #1408

Closed
DimitriPapadopoulos opened this issue May 13, 2024 · 4 comments · Fixed by #1414
Closed

Look into ruff rule RUF012 #1408

DimitriPapadopoulos opened this issue May 13, 2024 · 4 comments · Fixed by #1414

Comments

@DimitriPapadopoulos
Copy link
Contributor

How would this feature be useful?

RUF012 checks for mutable default values in class attributes:

Mutable default values share state across all instances of the class, while not being obvious. This can lead to bugs when the attributes are changed in one instance, as those changes will unexpectedly affect all other instances.

$ ruff check --extend-select RUF --ignore RUF005
src/pipx/animate.py:24:20: RUF012 Mutable class attributes should be annotated with `typing.ClassVar`
src/pipx/pipx_metadata_file.py:40:28: RUF012 Mutable class attributes should be annotated with `typing.ClassVar`
src/pipx/pipx_metadata_file.py:41:29: RUF012 Mutable class attributes should be annotated with `typing.ClassVar`
src/pipx/pipx_metadata_file.py:42:44: RUF012 Mutable class attributes should be annotated with `typing.ClassVar`
src/pipx/pipx_metadata_file.py:43:56: RUF012 Mutable class attributes should be annotated with `typing.ClassVar`
Found 5 errors.
$

Introduced in #610:

pipx/src/pipx/animate.py

Lines 23 to 24 in 5eed479

class _CursorInfo(ctypes.Structure):
_fields_ = [("size", ctypes.c_int), ("visible", ctypes.c_byte)]

Introduced in #222:

pipx/src/pipx/pipx_metadata_file.py

Lines 29 to 44 in 5eed479

class PackageInfo(NamedTuple):
package: Optional[str]
package_or_url: Optional[str]
pip_args: List[str]
include_dependencies: bool
include_apps: bool
apps: List[str]
app_paths: List[Path]
apps_of_dependencies: List[str]
app_paths_of_dependencies: Dict[str, List[Path]]
package_version: str
man_pages: List[str] = []
man_paths: List[Path] = []
man_pages_of_dependencies: List[str] = []
man_paths_of_dependencies: Dict[str, List[Path]] = {}
suffix: str = ""

Describe the solution you'd like

As a first step, I'd like to understand whether this is on purpose or not.

I suspect the relevant classes are designed to have one instance, in which case this is not an issue in practice. Nevertheless, it's good practice to avoid mutable default values, so that future maintainers don't have to ask themselves again.

Describe alternatives you've considered
@chrysle
Copy link
Contributor

chrysle commented May 13, 2024

I suspect the relevant classes are designed to have one instance, in which case this is not an issue in practice.

As for PackageInfo, there are several instances created for the different types of packages pipx recognizes (e.g. injected, main, ..). BTW, I think that should be made a dataclass.

Nevertheless, it's good practice to avoid mutable default values, so that future maintainers don't have to ask themselves again.

Agreed.

@huxuan
Copy link
Contributor

huxuan commented May 17, 2024

I think it is actually a bug, we did not encounter problem mostly because pipx is a cli tool and most functions are only called once before exit.

BTW, I notice that you have proposed many ruff rules, just wonder whehter there is any recommended rule set? AFAIK, there is no preset.

@DimitriPapadopoulos
Copy link
Contributor Author

DimitriPapadopoulos commented May 18, 2024 •
edited

While ruff enables the following set of rules by default, other rules are indeed useful:

By default, Ruff enables Flake8's F rules, along with a subset of the E rules, omitting any stylistic rules that overlap with the use of a formatter, like ruff format or Black.

It is hard to suggest a default set of rules. There is no wide enough consensus to balance the individual preferences of maintainers. I've seen maintainers revert from ruff to flake8 because ruff is not totally a drop-in replacement of flake8, and other maintainers embrace ruff and enable ALL rules except specific rules they disable.

The rules I have suggested so far reveal actual bugs or result undeniably(?) in more readable or faster code.

@DimitriPapadopoulos DimitriPapadopoulos mentioned this issue May 18, 2024
Merged
1 task
@huxuan
Copy link
Contributor

huxuan commented May 18, 2024

The rules I have suggested so far reveal actual bugs or result undeniably(?) in more readable or faster code.

Do not get me wrong, I am trying to learn about a recommended rule set. :-)

@DimitriPapadopoulos DimitriPapadopoulos mentioned this issue May 18, 2024
Merged
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Apply ruff rule RUF012 DimitriPapadopoulos/pipx
3 participants
@huxuan @DimitriPapadopoulos @chrysle