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

Organize imports and Ruff: Format imports apply other autofixes #328

Closed
trag1c opened this issue Nov 4, 2023 · 10 comments · Fixed by astral-sh/ruff-lsp#310
Closed

Organize imports and Ruff: Format imports apply other autofixes #328

trag1c opened this issue Nov 4, 2023 · 10 comments · Fixed by astral-sh/ruff-lsp#310
Labels
bug Something isn't working

Comments

@trag1c
Copy link

trag1c commented Nov 4, 2023

Screen.Recording.2023-11-04.at.11.02.26.mov
@charliermarsh
Copy link
Member

Can you share the relevant portion of your settings.json? Wasn't able to reproduce with my first attempt locally, but there could certainly be a bug here.

@trag1c
Copy link
Author

trag1c commented Nov 4, 2023

    "ruff.lint.args": [
      "--select=F,E,W,I,UP,N,S,C,B,A,T,Q,RUF,YTT,BLE,ANN,FBT,PL,TRY,RSE,SLF,DTZ,EXE,ISC,ICN,G,INP,PIE,RET,SIM,TID,TCH,ARG,PTH,PERF,SLOT",
      "--ignore=DTZ005,INP001,TCH003,ANN101,ANN102,ANN401,TID252,T201,B905,S101,TRY003,PLR2004"
    ],

@dhruvmanila
Copy link
Member

dhruvmanila commented Nov 6, 2023

I see the problem here. We should be using --select instead for the only keyword argument.

https://github.com/astral-sh/ruff-lsp/blob/a2661eff739c4b1dca4f18c73ef69bc90706dfaa/ruff_lsp/server.py#L1175-L1182

Edit: We would actually want a --force-select here to have exclusive selections.

@dhruvmanila dhruvmanila added the bug Something isn't working label Nov 6, 2023
@dhruvmanila
Copy link
Member

dhruvmanila commented Nov 6, 2023

Hmm. Actually we would also need to check if there's --extend-select argument in the user config and remove that before passing it to the Ruff CLI.

Edit: This would need to account for ["--extend-select=A,B,C"] and ["--extend-select", "A,B,C"].

@dhruvmanila
Copy link
Member

dhruvmanila commented Nov 6, 2023

Or, we could filter out the diagnostics to only keep the ones matching in the only list.

Edit: This won't be possible as we would directly get the fixed content, so there's no structured data to filter.

@charliermarsh
Copy link
Member

I thought we used argv += ["--extend-ignore", "ALL"] to solve this problem?

@dhruvmanila
Copy link
Member

It seems like --ignore=ALL isn't working:

$ ruff check --no-cache --isolated src/play.py --select=I,ANN,PT --ignore=ALL
src/play.py:1:1: I001 [*] Import block is un-sorted or un-formatted
src/play.py:5:9: ANN201 Missing return type annotation for public function `test_foo`
src/play.py:5:18: ANN101 Missing type annotation for `self` in method
src/play.py:6:9: PT009 Use a regular `assert` instead of unittest-style `assertTrue`
Found 4 errors.
[*] 1 fixable with the `--fix` option (1 hidden fix can be enabled with the `--unsafe-fixes` option).

But,

$ ruff check --no-cache --isolated src/play.py --select=I,ANN,PT --ignore=I,ANN   
src/play.py:6:9: PT009 Use a regular `assert` instead of unittest-style `assertTrue`
Found 1 error.
No fixes available (1 hidden fix can be enabled with the `--unsafe-fixes` option).

Version:

$ ruff version                                                    
ruff 0.1.4 (c4889196e 2023-11-03)

From the documentation, it seems like the correct behavior?

When breaking ties between enabled and disabled rules (via select and ignore, respectively), more specific prefixes override less specific prefixes.

@charliermarsh
Copy link
Member

@dhruvmanila - Yeah I think the use of --ignore=ALL won't work when users are specifying selects and ignores in their ruff.lint.args... Another option is we could attempt to filter out any custom --select or --ignore when running with only? Like literally filter out the array of arguments?

@dhruvmanila
Copy link
Member

Another option is we could attempt to filter out any custom --select or --ignore when running with only? Like literally filter out the array of arguments?

Yeah, that's likely the best option for now.

dhruvmanila added a commit to astral-sh/ruff-lsp that referenced this issue Nov 7, 2023
## Summary

Earlier, if a user had define a rule selection or exclusion using the
`lint.args` setting, it would collide with the `only` parameter. The
purpose of the `only` parameter is to only run Ruff for the given rule
excluding everything else. The "excluding everything else" part was done
with `--extend-ignore=ALL`. The problem, as highlighted in the linked
issue, was that if there's a user defined `--ignore` / `--select` (or
their `--extend-*` version), then the `ALL` directive won't work
because:

> When breaking ties between enabled and disabled rules (via select and
ignore, respectively), more specific prefixes override less specific
prefixes.
>
> _Reference: https://docs.astral.sh/ruff/settings/#ignore_

This means that between `--select=E,F` and `--ignore=ALL`, the former
wins.

This PR fixes this issue by checking for the following argument pattern
in user defined settings, skipping them if the `only` parameter is
given:

* `["--select", "A,B,C"]` / `["--select=A,B,C"]`
* `["--ignore", "A,B,C"]` / `["--ignore=A,B,C"]`

And their `--extend-*` version.

## Test Plan

Manual testing locally :)

fixes: astral-sh/ruff-vscode#328
@dhruvmanila
Copy link
Member

Thanks for the issue! This is fixed and will be available in the next release.

azurelotus0926 added a commit to azurelotus0926/ruff-lsp that referenced this issue Jun 27, 2024
## Summary

Earlier, if a user had define a rule selection or exclusion using the
`lint.args` setting, it would collide with the `only` parameter. The
purpose of the `only` parameter is to only run Ruff for the given rule
excluding everything else. The "excluding everything else" part was done
with `--extend-ignore=ALL`. The problem, as highlighted in the linked
issue, was that if there's a user defined `--ignore` / `--select` (or
their `--extend-*` version), then the `ALL` directive won't work
because:

> When breaking ties between enabled and disabled rules (via select and
ignore, respectively), more specific prefixes override less specific
prefixes.
>
> _Reference: https://docs.astral.sh/ruff/settings/#ignore_

This means that between `--select=E,F` and `--ignore=ALL`, the former
wins.

This PR fixes this issue by checking for the following argument pattern
in user defined settings, skipping them if the `only` parameter is
given:

* `["--select", "A,B,C"]` / `["--select=A,B,C"]`
* `["--ignore", "A,B,C"]` / `["--ignore=A,B,C"]`

And their `--extend-*` version.

## Test Plan

Manual testing locally :)

fixes: astral-sh/ruff-vscode#328
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants