astral-sh / ruff-vscode

A Visual Studio Code extension with support for the Ruff linter.
Other
1.12k stars 53 forks source link

useless error message when invalid settings in `ruff.toml` #642

Open DetachHead opened 3 days ago

DetachHead commented 3 days ago
[lint]
ignore = ["asdfasdf"]

this causes the following error message to appear:

Error while resolving settings from workspace c:\Users\user\project. Please refer to the logs for more details.

however there's nothing useful in the logs:

image

running ruff check in the cli shows the actual error:

> ruff check
ruff failed
  Cause: Failed to parse C:\Users\user\project\ruff.toml
  Cause: TOML parse error at line 6, column 1
  |
6 | [lint]
  | ^^^^^^
Unknown rule selector: `asdfasdf`

ideally this error would be included in the log

KotlinIsland commented 3 days ago

ideally this error would be included in the notification

dhruvmanila commented 3 days ago

Thanks for the issue. This is a deliberate choice from my end because of the way configuration works. The indexer runs in parallel and builds up the settings index collecting all the errors found during this process. Even if there are multiple files with an error, we'd only display a single popup notification and provide all the details in the output window. If we were to show a notification popup for every error, then it might spam the window with multiple of them.

One option would be to create a list of error messages and show the entire message in a single notification window but it might still create a rather large popup. You can already see that the message for a single error is 6 lines long. This was also the reason that we're providing the "Show Logs" button on the notification popup so that it's just 1-click away.

That said, I'm curious to know other's thoughts on this. I'm more than happy to reconsider if the latter is more useful from a user perspective. I chose this for a better UX :)

For reference, this is the method that I'm talking about: https://github.com/astral-sh/ruff/blob/924741cb11a68ed037899f9db1bea6969c48385e/crates/ruff_server/src/session/index/ruff_settings.rs#L103-L117

KotlinIsland commented 3 days ago

what about: if there is a single error, or the combination of all error messages is less than a couple lines, then show it in the notification, if there is extra, say the message is large and have the "show log" button?

MichaReiser commented 3 days ago

We should definitely fix that the message isn't included in the logs by default

dhruvmanila commented 3 days ago

@MichaReiser By message, do you mean this one: "Failed to parse C:\Users\user\project\ruff.toml" ?

MichaReiser commented 3 days ago

Yes, we should at least show a message similar to the ruff check output (in the output panel)

dhruvmanila commented 3 days ago

Oh I see the issue. You'll need to enable server messages by setting ruff.trace.server to messages for them to show up in the output channel. I want to address this in the coming weeks. Then, the output channel should basically contain the same message as the CLI. This is a known limitation in the server logging.