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

PIE790 fix can make non-docstrings into docstrings #12616

Closed
dscorbett opened this issue Aug 1, 2024 · 2 comments · Fixed by #14393
Closed

PIE790 fix can make non-docstrings into docstrings #12616

dscorbett opened this issue Aug 1, 2024 · 2 comments · Fixed by #14393
Labels
fixes Related to suggested fixes for violations help wanted Contributions especially welcome rule Implementing or modifying a lint rule

Comments

@dscorbett
Copy link

There is one case where the fix for PIE790 is not completely safe: it changes behavior when the placeholder statement is blocking a string from being a docstring.

$ ruff --version
ruff 0.5.5
$ cat pie790.py
pass
"docstring?"
print(f"{__doc__=}")
$ python pie790.py
__doc__=None
$ ruff check --isolated --fix --select PIE790 pie790.py
Found 1 error (1 fixed, 0 remaining).
$ python pie790.py
__doc__='docstring?'
@MichaReiser
Copy link
Member

Interesting. It shouldn't be to hard to not flag the rule in this case. Just wondering, what's the real world code where you found this issue because a string literal probably indicates a missing assignment (or return, ...).?

@MichaReiser MichaReiser added rule Implementing or modifying a lint rule help wanted Contributions especially welcome labels Aug 1, 2024
@dscorbett
Copy link
Author

This is just something theoretical I noticed while reading the rule description. I agree that anything that looks like my sample code probably has a bug. I think PIE790 should still flag it, just not automatically fix it.

@AlexWaygood AlexWaygood added the fixes Related to suggested fixes for violations label Aug 2, 2024
charliermarsh added a commit that referenced this issue Nov 18, 2024
…ing literal (`PIE790`) (#14393)

## Summary

Resolves #12616.

## Test Plan

`cargo nextest run` and `cargo insta test`.

---------

Co-authored-by: Charlie Marsh <[email protected]>
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 help wanted Contributions especially welcome rule Implementing or modifying a lint rule
Projects
None yet
3 participants