Warn against certain values in notebook.codeActionsOnSave
#686
Conversation
dhruvmanila
force-pushed
the
dhruv/warn-notebook-ca
branch
from
fdbbfc0 to
8119f38
Compare
src/common/settings.ts
Outdated
| if (genericCodeActions.length > 0) { | ||
| // This is at info level because other extensions might be using these code actions but we still want to inform the user. | ||
| logger.info( | ||
| `The following code actions in 'notebook.codeActionsOnSave' could lead to unexpected behavior: ${JSON.stringify( | ||
| genericCodeActions, | ||
| )}. Consider using ${JSON.stringify( | ||
| genericCodeActions.map((action) => `notebook.${action}`), | ||
| )} instead.`, | ||
| ); | ||
| } |
There was a problem hiding this comment.
I'm a bit unsure if we even want this but a lot of users seems to be using the ones without .ruff suffix so at least the logs could help them and us.
I'm also thinking of updating the README to group config to avoid a long list of code snippets which are mostly similar and it's hard to know which config should a user pickup from them.
src/common/settings.ts
Outdated
| logger.info( | ||
| `The following code actions in 'notebook.codeActionsOnSave' could lead to unexpected behavior: ${JSON.stringify( | ||
| genericCodeActions, | ||
| )}. Consider using ${JSON.stringify( | ||
| genericCodeActions.map((action) => `notebook.${action}`), | ||
| )} instead.`, | ||
| ); |
There was a problem hiding this comment.
It seems you also added a similar check on the LSP side. Do we only include the warning here because this is a VS code specific fix or is this something we could handle on the LSP side?
Should we include a link to some documentation explaining why? For example, we could add an FAQ entry or a section on the website explaining why using notebook-specific actions is preferred. It might also be could to link to the issue, but that's something we could do in the FAQ section where we have a bit more space than here.
There was a problem hiding this comment.
It seems you also added a similar check on the LSP side. Do we only include the warning here because this is a VS code specific fix or is this something we could handle on the LSP side?
Yes, this is VS Code specific fix as we're looking into notebook.codeActionsOnSave which specific to VS Code and can provide suggestion to change it.
Using editor specific fix allows us to provide a notification popup which when implemented on the server side would result in multiple popups because the client sends the code action request whenever cursor position is changed.
Should we include a link to some documentation explaining why? For example, we could add an FAQ entry or a section on the website explaining why using notebook-specific actions is preferred. It might also be could to link to the issue, but that's something we could do in the FAQ section where we have a bit more space than here.
Thanks, that's a great suggestion. I'll add that first.
## Summary Related to astral-sh/ruff-vscode#686, this PR ignores handling source code actions for notebooks which are not prefixed with `notebook`. The main motivation is that the native server does not actually handle it well which results in gibberish code. There's some context about this in astral-sh/ruff-vscode#680 (comment) and the following comments. closes: astral-sh/ruff-vscode#680 ## Test Plan Running a notebook with the following does nothing except log the message: ```json "notebook.codeActionsOnSave": { "source.organizeImports.ruff": "explicit", }, ``` while, including the `notebook` code actions does make the edit (as usual): ```json "notebook.codeActionsOnSave": { "notebook.source.organizeImports.ruff": "explicit" }, ```
Summary
This PR adds a check for
notebook.codeActionsOnSaveto do the following:source.fixAllandsource.organizeImports, log at info levelsource.fixAll.ruffandsource.organizeImports.ruff, log at warn level and provide a warning message to usenotebook.prefixed code actions insteadTest Plan
Using the following settings:
{ "notebook.codeActionsOnSave": { "source.fixAll": "explicit", "source.organizeImports": "explicit", "source.organizeImports.ruff": "explicit", "source.fixAll.ruff": "explicit" } }Logs:
Notification preview: