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
[pyupgrade
]: Remove outdated sys.version_info
blocks
#2099
Conversation
@charliermarsh I am hitting a blocker and was wondering if you could offer some assistance. My issue is linting the code below:
Currently, my program lints every line, and removes it if the condition meets the requirements, this means we end up with the following:
As you can see, this is not a valid python statement. I tried creating checkers that either look at the parent, or see if the statement is an if to remove the "el" from the "elif" in the next line, however; since ruff goes through all elif and if statements before running again, the code is already broken before the next loop through. So essentially I need to know whether to convert an elif statement to an if statement. Pyupgrade currently solves this issue by only removing one if or elif at a time, so to fix the statement above you would have to call pyupgrade four times. Only running on if statements is not a viable solution here because sometimes the issue might only be on an elif statement. So far my best ideas would be to:
|
It looks like I have a second issue that could be easily fixed if I can skip fixing the rest of the statement until the next iteration. Is there any functionality like that? |
We avoid applying fixes that "overlap" in the text. So one thing you could do is... when you go to fix one of these, generate a fix that applies to the entire So if you had this: if sys.version_info < (2,0):
print("This script requires Python 2.0 or greater.")
elif sys.version_info < (2,6):
print("This script requires Python 2.6 or greater.")
elif sys.version_info < (3,):
print("This script requires Python 3.0 or greater.")
elif sys.version_info < (3,0):
print("This script requires Python 3.0 or greater.")
elif sys.version_info < (3,12):
print("Python 3.0 or 3.1 or 3.2")
else:
print("Python 3") Then to fix the first one, you'd generate a if sys.version_info < (2,6):
print("This script requires Python 2.6 or greater.")
elif sys.version_info < (3,):
print("This script requires Python 3.0 or greater.")
elif sys.version_info < (3,0):
print("This script requires Python 3.0 or greater.")
elif sys.version_info < (3,12):
print("Python 3.0 or 3.1 or 3.2")
else:
print("Python 3") All the fixes will overlap, and so it'll only apply one per iteration. |
Charlie this is absolutely perfect! Thank you so much!!! |
No worries, sorry it took me a while to respond. It might take some work to find the outermost |
@charliermarsh, I believe I found an issue with the
However, when I attempt to use the lexer to get the tokens in the statement I get the following issue:
Based on this, and the position of the location where the error occurs, it looks like the parser is failing one the |
I think you need to dedent the entire block before lexing. Right now, you're feeding it: if six.PY2:
print("PY2")
else:
print("PY3") ...since you're starting at the (This is a guess, I admittedly didn't look at the code.) |
I think you are exactly right, is there an example somewhere in the code of this being done already? I noticed dedent does not work if the first line has no indent. |
pyupgrade
]: Remove outdated sys.version_info
blocks
src/rules/pyupgrade/mod.rs
Outdated
fn rules(rule_code: Rule, path: &Path) -> Result<()> { | ||
let snapshot = format!("{}_{}", rule_code.code(), path.to_string_lossy()); | ||
let snapshot = format!("{}_{}", rule_code.code(), path.display()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sbrugman - Do you have any idea why these are failing on Windows? I tried with both to_string_lossy()
and display()
but no luck.
@colin99d - Did a pass over this, I was able to simplify a few things a little bit, but I also made it slightly more timid in some cases... Do you have any interest in looking through or even testing the code prior to merging? |
Would love to! I can do this tonight. |
What is the reason for not refactoring single line if statements anymore? I am assuming there were some edge cases it was not handling well. |
I thought it would just be simpler to not worry about them given that they're pretty rare (e.g. Black doesn't preserve them). It's possible that the implementation was totally sound... I just get worried about cases like multi-statement lines and continuations: if True: a = 1; b = 2
if True: a = 1; \
b = 2 But this isn't a rigorous answer at all. Maybe we should revisit. |
I have not seen this in practice before EVER, so maybe we ignore it, and if someone leaves an issue, and it gains some traction, we can invest into it. |
There is one failing case right now, which I think we can only solve with LibCST -- something like this: if True:
if sys.version_info >= (3, 9):
expected_error = [
"<stdin>:1:5: Generator expression must be parenthesized",
"max(1 for i in range(10), key=lambda x: x+1)",
" ^",
]
elif PYPY:
expected_error = []
else:
expected_error = [] Or even harder: if True:
if sys.version_info >= (3, 9):
"""this
is valid"""
"""the indentation on
this line is significant"""
"this is" \
"allowed too"
("so is"
"this for some reason")
|
(Fixed.) |
Ok, I think this is good to go. |
@colin99d - LMK if you wanna give this a read tonight or if I should go ahead and merge. It's not urgent -- I probably won't release tonight anyway. |
Merging for now but LMK if you have any follow-up feedback. |
Sorry, had something come up last night. Looks good to me!!! |
All good, no prob at all! |
[![Mend Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com) This PR contains the following updates: | Package | Change | Age | Adoption | Passing | Confidence | |---|---|---|---|---|---| | [ruff](https://togithub.com/charliermarsh/ruff) | `^0.0.239` -> `^0.0.240` | [![age](https://badges.renovateapi.com/packages/pypi/ruff/0.0.240/age-slim)](https://docs.renovatebot.com/merge-confidence/) | [![adoption](https://badges.renovateapi.com/packages/pypi/ruff/0.0.240/adoption-slim)](https://docs.renovatebot.com/merge-confidence/) | [![passing](https://badges.renovateapi.com/packages/pypi/ruff/0.0.240/compatibility-slim/0.0.239)](https://docs.renovatebot.com/merge-confidence/) | [![confidence](https://badges.renovateapi.com/packages/pypi/ruff/0.0.240/confidence-slim/0.0.239)](https://docs.renovatebot.com/merge-confidence/) | --- ### Release Notes <details> <summary>charliermarsh/ruff</summary> ### [`v0.0.240`](https://togithub.com/charliermarsh/ruff/releases/tag/v0.0.240) [Compare Source](https://togithub.com/charliermarsh/ruff/compare/v0.0.239...v0.0.240) <!-- Release notes generated using configuration in .github/release.yml at main --> #### What's Changed ##### Rules - \[`pyupgrade`]: Remove outdated `sys.version_info` blocks by [@​colin99d](https://togithub.com/colin99d) in [astral-sh/ruff#2099 - \[`flake8-self`] Add Plugin and Rule `SLF001` by [@​saadmk11](https://togithub.com/saadmk11) in [astral-sh/ruff#2470 - \[`pylint`] Implement pylint's `too-many-statements` rule (`PLR0915`) by [@​chanman3388](https://togithub.com/chanman3388) in [astral-sh/ruff#2445 ##### Settings - \[`isort`] Support forced_separate by [@​akx](https://togithub.com/akx) in [astral-sh/ruff#2268 - \[`isort`] Add isort option lines-after-imports by [@​reidswan](https://togithub.com/reidswan) in [astral-sh/ruff#2440 - Allow non-ruff.toml-named files for --config by [@​charliermarsh](https://togithub.com/charliermarsh) in [astral-sh/ruff#2463 ##### Bug Fixes - Trigger, but don't fix, SIM rules if comments are present by [@​charliermarsh](https://togithub.com/charliermarsh) in [astral-sh/ruff#2450 - Only avoid PEP604 rewrites for pre-Python 3.10 code by [@​charliermarsh](https://togithub.com/charliermarsh) in [astral-sh/ruff#2449 - Use LibCST to reverse Yoda conditions by [@​charliermarsh](https://togithub.com/charliermarsh) in [astral-sh/ruff#2468 - fix: assertTrue()/assertFalse() fixer should not test for identity by [@​spaceone](https://togithub.com/spaceone) in [astral-sh/ruff#2476 - Treat `if 0:` and `if False:` as type-checking blocks by [@​charliermarsh](https://togithub.com/charliermarsh) in [astral-sh/ruff#2485 - Avoid iterating over body twice by [@​charliermarsh](https://togithub.com/charliermarsh) in [astral-sh/ruff#2439 - more builtin name checks when autofixing by [@​spaceone](https://togithub.com/spaceone) in [astral-sh/ruff#2430 - Respect parent noqa in --add-noqa by [@​charliermarsh](https://togithub.com/charliermarsh) in [astral-sh/ruff#2464 - Avoid removing un-selected codes when applying `--add-noqa` edits by [@​charliermarsh](https://togithub.com/charliermarsh) in [astral-sh/ruff#2465 - Carry-over `ignore` to next config layer if `select = []` by [@​not-my-profile](https://togithub.com/not-my-profile) in [astral-sh/ruff#2467 - Visit NamedExpr values before targets by [@​charliermarsh](https://togithub.com/charliermarsh) in [astral-sh/ruff#2484 #### New Contributors - [@​reidswan](https://togithub.com/reidswan) made their first contribution in [astral-sh/ruff#2440 - [@​chanman3388](https://togithub.com/chanman3388) made their first contribution in [astral-sh/ruff#2445 **Full Changelog**: astral-sh/ruff@v0.0.239...v0.0.240 </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Enabled. ♻ **Rebasing**: Whenever PR is behind base branch, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box --- This PR has been generated by [Mend Renovate](https://www.mend.io/free-developer-tools/renovate/). View repository job log [here](https://app.renovatebot.com/dashboard#github/ixm-one/pytest-cmake-presets). <!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNC4xMTkuNSIsInVwZGF0ZWRJblZlciI6IjM0LjExOS41In0=--> Signed-off-by: Renovate Bot <bot@renovateapp.com> Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
A part of #827. Opening this up as a draft for visibility.