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

Ignore source code actions for a notebook cell #16154

Merged
merged 2 commits into from
Feb 18, 2025

Conversation

dhruvmanila
Copy link
Member

@dhruvmanila dhruvmanila commented Feb 14, 2025

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:

  "notebook.codeActionsOnSave": {
    "source.organizeImports.ruff": "explicit",
  },

while, including the notebook code actions does make the edit (as usual):

  "notebook.codeActionsOnSave": {
    "notebook.source.organizeImports.ruff": "explicit"
  },

@dhruvmanila dhruvmanila added the server Related to the LSP server label Feb 14, 2025
Copy link
Contributor

github-actions bot commented Feb 14, 2025

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

@dhruvmanila dhruvmanila force-pushed the dhruv/ignore-source-code-action-for-cell branch from 4a07a84 to 242de7a Compare February 14, 2025 13:39
@dhruvmanila dhruvmanila added the bug Something isn't working label Feb 14, 2025
@dhruvmanila dhruvmanila marked this pull request as ready for review February 14, 2025 13:44
@@ -48,7 +48,11 @@ impl super::BackgroundDocumentRequestHandler for CodeActions {

if snapshot.client_settings().fix_all() {
if supported_code_actions.contains(&SupportedCodeAction::SourceFixAll) {
response.push(fix_all(&snapshot).with_failure_code(ErrorCode::InternalError)?);
if snapshot.is_notebook_cell() {
tracing::debug!("Ignoring `source.fixAll` code action for a notebook cell");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add some inline comment explaining why we're ignoring those actions or link to the relevant issue? It's otherwise somewhat confusing when reading this in the future.

Related: Should we use a warn or info severity here so that it's visible by default? Should the message include a hint of what the user should do instead or is this expected inside of notebooks?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add some inline comment explaining why we're ignoring those actions or link to the relevant issue? It's otherwise somewhat confusing when reading this in the future.

Yes, will do so.

Related: Should we use a warn or info severity here so that it's visible by default? Should the message include a hint of what the user should do instead or is this expected inside of notebooks?

The reason I'm not using a warn or info severity is that this message will quickly fill up the log file because the code actions are request for cursor movements and possibly even while typing.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Regarding the hint, I'm not even sure if there are any editors other than VS Code-like ones that support notebooks natively along with the LSP capabilities so I'm hesitant to provide any suggestions here.

@dhruvmanila dhruvmanila force-pushed the dhruv/ignore-source-code-action-for-cell branch from 242de7a to f7425ab Compare February 17, 2025 13:01
@dhruvmanila dhruvmanila merged commit 82eae51 into main Feb 18, 2025
21 checks passed
@dhruvmanila dhruvmanila deleted the dhruv/ignore-source-code-action-for-cell branch February 18, 2025 04:58
dcreager added a commit that referenced this pull request Feb 18, 2025
* main: (60 commits)
  [`refurb`] Manual timezone monkeypatching (`FURB162`) (#16113)
  [`pyupgrade`] Do not upgrade functional TypedDicts with private field names to the class-based syntax (`UP013`) (#16219)
  Improve docs for PYI019 (#16229)
  Refactor `CallOutcome` to `Result` (#16161)
  Fix minor punctuation errors (#16228)
  Include document specific debug info (#16215)
  Update server to return the debug info as string (#16214)
  [`airflow`] Group `ImportPathMoved` and `ProviderName` to avoid misusing (`AIR303`) (#16157)
  Fix unstable formatting of trailing end-of-line comments of parenthesized attribute values (#16187)
  Ignore source code actions for a notebook cell (#16154)
  Add FAQ entry for `source.*` code actions in Notebook (#16212)
  red-knot: move symbol lookups in `symbol.rs` (#16152)
  better error messages while loading configuration `extend`s (#15658)
  Format `index.css` (#16207)
  Improve API exposed on `ExprStringLiteral` nodes (#16192)
  Update Rust crate tempfile to v3.17.0 (#16202)
  Update cloudflare/wrangler-action action to v3.14.0 (#16203)
  Update NPM Development dependencies (#16199)
  Update Rust crate smallvec to v1.14.0 (#16201)
  Update Rust crate codspeed-criterion-compat to v2.8.0 (#16200)
  ...
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 this pull request may close these issues.

Formatting imports in notebook in VSCode injects random code into cell
2 participants