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

lint.per-file-ignores option is ignored by the new language server #11751

Closed
DetachHead opened this issue Jun 5, 2024 · 10 comments · Fixed by #11800
Closed

lint.per-file-ignores option is ignored by the new language server #11751

DetachHead opened this issue Jun 5, 2024 · 10 comments · Fixed by #11800
Assignees
Labels
bug Something isn't working server Related to the LSP server

Comments

@DetachHead
Copy link

when enabling the new ruff.nativeServer setting in vscode, it seems to ignore tool.ruff.lint.per-file-ignores

# pyproject.toml

[tool.ruff.lint]
extend-select = ["S101"]
[tool.ruff.lint.per-file-ignores]
"tests/**/.py" = ["S101"]
// .vscode/settings.json

{
  "ruff.nativeServer": true
}

image

@MichaReiser MichaReiser added bug Something isn't working server Related to the LSP server labels Jun 5, 2024
@charliermarsh
Copy link
Member

I think you have a typo: "tests/**/.py" should be `"tests/**/*.py". Otherwise, it works in my testing.

@DetachHead
Copy link
Author

oh oops! weird though because the old language server and the cli both seem to work with that glob

@DetachHead
Copy link
Author

fixing that typo doesn't seem to work for me. it seems that the new lsp is only seeing the name of the file, but not the full path. "foo.py" works but "tests/foo.py" doesn't

@charliermarsh
Copy link
Member

charliermarsh commented Jun 6, 2024

I'm not sure we can do. In the ruff-vscode repo, I added:

[tool.ruff.lint.per-file-ignores]
"tests/**/*.py" = ["F401"]

And F401 errors in ./tests/client/constants.py are successfully ignored, as are errors in ./tests/test_server.py.

@DetachHead
Copy link
Author

i think it's because that repo does not use the new language server. adding the same thing to pyproject.toml, then adding "ruff.nativeServer": true to .vscode/settings.json causes the error to appear.

@charliermarsh
Copy link
Member

Not for me... I have it enabled in my user settings, and I've verified that it's running via the logging. Could you be using an old Ruff version? There were some early versions of the native server that didn't support this.

@DetachHead
Copy link
Author

according to the vscode extension log it's using 0.4.8:

2024-06-07 10:10:38.188 [info] Found ruff 0.4.8 at c:\Users\user\.vscode\extensions\charliermarsh.ruff-2024.26.0-win32-x64\bundled\libs\bin\ruff.exe

maybe it only happens on windows? i'm on windows 10, let me know if you need any more info

@charliermarsh
Copy link
Member

I will try it on Windows, perhaps there's a difference there.

@charliermarsh
Copy link
Member

Ok, I can successfully reproduce this on Windows.

@charliermarsh
Copy link
Member

So the LSP file paths are now like:

/c%3A/Users/crmar/workspace/fastapi/tests/main.py

But the resolved ignore patterns are like:

c:\\Users\\crmar\\workspace\\fastapi\\tests\\**\\*.py

@charliermarsh charliermarsh self-assigned this Jun 8, 2024
snowsignal pushed a commit that referenced this issue Jun 8, 2024
## Summary

As-is, we're using the URL path for all files, leading us to use paths
like:

```
/c%3A/Users/crmar/workspace/fastapi/tests/main.py
```

This doesn't match against per-file ignores and other patterns in Ruff
configuration.

This PR modifies the LSP to use the real file path if available, and the
virtual file path if not.

Closes #11751.

## Test Plan

Ran the LSP on Windows. In the FastAPI repo, added:

```toml
[tool.ruff.lint.per-file-ignores]
"tests/**/*.py" = ["F401"]
```

And verified that an unused import was ignored in `tests` after this
change, but not before.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working server Related to the LSP server
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants