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

[PLW1507] Mark fix unsafe #16343

Merged
merged 2 commits into from
Feb 24, 2025
Merged

[PLW1507] Mark fix unsafe #16343

merged 2 commits into from
Feb 24, 2025

Conversation

VascoSch92
Copy link
Contributor

This PR addresses issue #16274

Just marked the fix as unsafe, as it is not possible to check when a fix is safe or not (see above cited issue for example).

Copy link
Contributor

github-actions bot commented Feb 24, 2025

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

@MichaReiser MichaReiser added the fixes Related to suggested fixes for violations label Feb 24, 2025
Copy link
Member

@MichaReiser MichaReiser left a comment

Choose a reason for hiding this comment

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

Thank you

We should add some documentation explaining why the fix is unsafe. You can search for ## Fix safety to see similar comments for other rules.

Comment on lines 1 to 9
# Test case where the proposed fix is wrong, i.e., unsafe fix
# Ref: https://github.com/astral-sh/ruff/issues/16274#event-16423475135

import copy
import os

os.environ["X"] = "0"
env = copy.copy(os.environ)
os.environ["X"] = "1"
Copy link
Member

Choose a reason for hiding this comment

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

I think we could keep this test case in the same file but add a comment explaining what it does.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes!

@VascoSch92
Copy link
Contributor Author

Thank you

We should add some documentation explaining why the fix is unsafe. You can search for ## Fix safety to see similar comments for other rules.

I added the explanation of the unsafe fix. I don't know if it would help to add a small example or/and quoting the issue with the example why the fix is unsafe

Copy link
Member

@MichaReiser MichaReiser left a comment

Choose a reason for hiding this comment

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

This is perfect, thank you

@MichaReiser MichaReiser merged commit 42a5f5e into astral-sh:main Feb 24, 2025
21 checks passed
dcreager added a commit that referenced this pull request Feb 25, 2025
* main: (38 commits)
  [red-knot] Use arena-allocated association lists for narrowing constraints (#16306)
  [red-knot] Rewrite `Type::try_iterate()` to improve type inference and diagnostic messages (#16321)
  Add issue templates (#16213)
  Normalize inconsistent markdown headings in docstrings (#16364)
  [red-knot] Better diagnostics for method calls (#16362)
  [red-knot] Add argfile and windows glob path support (#16353)
  [red-knot] Handle pipe-errors gracefully (#16354)
  Rename `venv-path` to `python` (#16347)
  [red-knot] Fixup some formatting in `infer.rs` (#16348)
  [red-knot] Restrict visibility of more things in `class.rs` (#16346)
  [red-knot] Add diagnostic for class-object access to pure instance variables (#16036)
  Add `per-file-target-version` option (#16257)
  [PLW1507] Mark fix unsafe (#16343)
  [red-knot] Add a test to ensure that `KnownClass::try_from_file_and_name()` is kept up to date (#16326)
  Extract class and instance types (#16337)
  Re-order changelog entries for 0.9.7 (#16344)
  [red-knot] Add support for `@classmethod`s (#16305)
  Update Salsa (#16338)
  Update Salsa part 1 (#16340)
  Upgrade Rust toolchain to 1.85.0 (#16339)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fixes Related to suggested fixes for violations
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants