Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

collapsible-else-if (PLR5501) --fix removes comments #13545

Closed
sylvestre opened this issue Sep 28, 2024 · 6 comments · Fixed by #13573
Closed

collapsible-else-if (PLR5501) --fix removes comments #13545

sylvestre opened this issue Sep 28, 2024 · 6 comments · Fixed by #13573
Labels
fixes Related to suggested fixes for violations

Comments

@sylvestre
Copy link

Trying to reformat firefox code with ruff https://phabricator.services.mozilla.com/D223851#inline-1243679
we noticed a bug with collapsible-else-if - https://docs.astral.sh/ruff/rules/collapsible-else-if/#collapsible-else-if-plr5501

take this case:

def check_sign(value: int) -> None:
    if value > 0:
        print("Number is positive.")
    else:
	# my comment to explain this
	# but will be removed
        if value < 0:
            print("Number is negative.")
        else:
            print("Number is zero.")

Then, with ruff main (ec72e67)

cargo run -p ruff -- check --select PLR5501 foo.py --no-cache  --fix  
Found 1 error (1 fixed, 0 remaining).

will update the code to:

def check_sign(value: int) -> None:
    if value > 0:
        print("Number is positive.")
    elif value < 0:
        print("Number is negative.")
    else:
        print("Number is zero.")

which removed my comment

@sylvestre
Copy link
Author

cc @diceroll123 as it seems that you are the original author

@MichaReiser
Copy link
Member

Related to #9790

@MichaReiser
Copy link
Member

@sylvestre where would you expect the comment to be placed after the fix?

@MichaReiser MichaReiser added the fixes Related to suggested fixes for violations label Sep 28, 2024
@sylvestre
Copy link
Author

Good point.
To keep it relevant, I would continue on the line #elif and move on the next line

@zanieb
Copy link
Member

zanieb commented Sep 30, 2024

We have test coverage for this behavior already at

52 52 | def not_ok1_with_comments():
53 53 | if 1:
54 54 | pass
55 |+ elif 2:
56 |+ pass
55 57 | else:
56 |- # inner comment
57 |- if 2:
58 |- pass
59 |- else:
60 |- pass # final pass comment
58 |+ pass # final pass comment

zanieb added a commit that referenced this issue Oct 1, 2024
…13572)

While looking into #13545 I
noticed that we return `None` here if you pass a block of comments. This
is annoying because it causes `adjust_indentation` to fall back to
LibCST which panics when it cannot find a statement.
zanieb added a commit that referenced this issue Oct 3, 2024
Closes #13545

As described in the issue, we move comments before the inner `if`
statement to before the newly constructed `elif` statement (previously
`else`).
@sylvestre
Copy link
Author

Thanks!

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
fixes Related to suggested fixes for violations
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants