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

Improve deprecation warnings #685

Merged
merged 2 commits into from
Feb 10, 2025
Merged

Conversation

dhruvmanila
Copy link
Member

@dhruvmanila dhruvmanila commented Feb 9, 2025

Summary

Related: astral-sh/ruff-lsp#522

Test Plan

1. User explicitly sets native server "on" with legacy settings

settings.json:

{
	"ruff.nativeServer": "on",
	"ruff.lint.args": []
}

Logs:

2025-02-10 10:47:34.157 [warning] Following settings have been deprecated in the native server: ["ruff.lint.args"]. Please [migrate](https://docs.astral.sh/ruff/editors/migration/) to the new settings or remove them. Feel free to comment on the [GitHub discussion](https://github.com/astral-sh/ruff/discussions/15991) to ask questions or share feedback.

Notification:

Screenshot 2025-02-10 at 10 47 44 AM

2. User explicitly sets native server "off" with no legacy settings

settings.json:

{
	"ruff.nativeServer": "off"
}

Logs:

2025-02-10 10:49:26.272 [warning] Legacy server ([ruff-lsp](https://github.com/astral-sh/ruff-lsp)) has been deprecated. Please consider using the native server instead by removing 'ruff.nativeServer' or setting it to 'on'. Feel free to comment on the [GitHub discussion](https://github.com/astral-sh/ruff/discussions/15991) to ask questions or share feedback.

Notification:

Screenshot 2025-02-10 at 10 49 30 AM

3. User explicitly sets native server "off" with legacy settings

settings.json:

{
	"ruff.nativeServer": "off",
	"ruff.lint.args": []
}

Logs:

2025-02-10 10:50:44.897 [warning] Legacy server ([ruff-lsp](https://github.com/astral-sh/ruff-lsp)) has been deprecated. Please consider using the native server instead by removing 'ruff.nativeServer' or setting it to 'on'. Following settings are not supported with the native server and have been deprecated: ["ruff.lint.args"]. Please [migrate](https://docs.astral.sh/ruff/editors/migration/) to the new settings or remove them. Feel free to comment on the [GitHub discussion](https://github.com/astral-sh/ruff/discussions/15991) to ask questions or share feedback.

Notification:
Screenshot 2025-02-10 at 10 50 53 AM

4. Auto behavior with legacy settings

settings.json:

{
	"ruff.nativeServer": "auto",
	"ruff.lint.args": []
}

Logs:

2025-02-10 10:52:07.449 [warning] Legacy server ([ruff-lsp](https://github.com/astral-sh/ruff-lsp)) has been deprecated. Please consider using the native server instead by setting 'ruff.nativeServer' to 'on'. Following settings are not supported with the native server and have been deprecated: ["ruff.lint.args"]. Please [migrate](https://docs.astral.sh/ruff/editors/migration/) to the new settings or remove them. Feel free to comment on the [GitHub discussion](https://github.com/astral-sh/ruff/discussions/15991) to ask questions or share feedback.

Notification:
Screenshot 2025-02-10 at 10 52 13 AM

5. Auto behavior with non-stable ruff server

Same as (4) with Ruff 0.5.2 installed. The notification message is the same except that the logs include an additional mesage:

2025-02-10 11:22:56.654 [info] Stable version of the native server requires Ruff 0.5.3, but found 0.5.2 at /Users/dhruv/.local/bin/ruff instead
2025-02-10 11:22:56.654 [warning] Legacy server ([ruff-lsp](https://github.com/astral-sh/ruff-lsp)) has been deprecated. Please consider using the native server instead by setting 'ruff.nativeServer' to 'on'. Following settings are not supported with the native server and have been deprecated: ["ruff.lint.args"]. Please [migrate](https://docs.astral.sh/ruff/editors/migration/) to the new settings or remove them. Feel free to comment on the [GitHub discussion](https://github.com/astral-sh/ruff/discussions/15991) to ask questions or share feedback.

6. Auto behavior with Ruff without the server command

This uses Ruff 0.3.4 and will first provide a warning message same as (4) and when the user sets nativeServer to on, they'll get an error:

Screenshot 2025-02-10 at 11 25 51 AM

@dhruvmanila dhruvmanila force-pushed the dhruv/improve-deprecation-warnings branch from b4c303b to 2cd9620 Compare February 10, 2025 05:56
@dhruvmanila dhruvmanila marked this pull request as ready for review February 10, 2025 05:57
@dhruvmanila
Copy link
Member Author

I'm a bit unsure on the flow of (5) and (6), any suggestions are appreciated.

@MichaReiser
Copy link
Member

For 4: I find it a bit confusing that we suggest setting to on nativeServer: on when it is sufficient for them to migrate the settings. Requiring explicitly setting nativeServer: on does have the downside that many users never set nativeServer: auto, it's just the default (or did we change the default)?

For 5: I'm unsure what the info message is trying to tell me. Does it mean I have to upgrade to use the native server or is it only a hint that I'd be using an experimental version if I keep using the older version?

6 doesn't seem ideal but it's also not terrible. Maybe we could add a note to the migration guide that using the native server requires at least Ruff 0.3.5 to give users a heads up?

@dhruvmanila
Copy link
Member Author

Thank you for the feedback.

For 4: I find it a bit confusing that we suggest setting to on nativeServer: on when it is sufficient for them to migrate the settings. Requiring explicitly setting nativeServer: on does have the downside that many users never set nativeServer: auto, it's just the default (or did we change the default)?

Yes, "auto" is the default value. I'll remove the nativeServer: on suggestion.

For 5: I'm unsure what the info message is trying to tell me. Does it mean I have to upgrade to use the native server or is it only a hint that I'd be using an experimental version if I keep using the older version?

That message already existed, but I think I can improve it:

Stable version of the native server requires Ruff 0.5.3, but found 0.5.2 at /Users/dhruv/.local/bin/ruff instead; using the legacy server (ruff-lsp)

Note that there's also this info message that'll be present so overall it says that the auto behavior found the Ruff version which does not include stable server so will use ruff-lsp instead:

Resolved 'ruff.nativeServer: auto' to use the legacy (ruff-lsp) server

6 doesn't seem ideal but it's also not terrible. Maybe we could add a note to the migration guide that using the native server requires at least Ruff 0.3.5 to give users a heads up?

Yeah, that makes sense.

dhruvmanila added a commit to astral-sh/ruff-lsp that referenced this pull request Feb 10, 2025
## Summary

Related: astral-sh/ruff-vscode#685

This PR removes the deprecation warning for VS Code client and removes
the version check as well as it's not needed for other editors.

## Test Plan

While testing astral-sh/ruff-vscode#685 with
local `ruff-lsp` on this branch, VS Code should only provides
notification from the extension and not the one from `ruff-lsp`.
@dhruvmanila dhruvmanila merged commit 3b4d54d into main Feb 10, 2025
6 checks passed
@dhruvmanila dhruvmanila deleted the dhruv/improve-deprecation-warnings branch February 10, 2025 09:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants