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

Fix automatic configuration reloading for text and notebook documents #11492

Merged
merged 2 commits into from
May 22, 2024

Conversation

snowsignal
Copy link
Contributor

@snowsignal snowsignal commented May 22, 2024

Summary

Recent changes made in the Jupyter Notebook feature PR caused automatic configuration reloading to stop working. This was because we would check for paths to reload using the changed path, when we should have been using the parent path of the changed path (to get the directory it was changed in).

Additionally, this PR fixes an issue where ruff.toml and .ruff.toml files were not being automatically reloaded.

Finally, this PR improves configuration reloading by actively publishing diagnostics for notebook documents (which won't be affected by the workspace refresh since they don't use pull diagnostics). It will also publish diagnostics for text documents if pull diagnostics aren't supported.

Test Plan

To test this, open an existing configuration file in a codebase, and make modifications that will affect one or more open Python / Jupyter Notebook files. You should observe that the diagnostics for both kinds of files update automatically when the file changes are saved.

Here's a test video showing what a successful test should look like:

Screen.Recording.2024-05-22.at.11.13.18.AM.mov

…notebooks (and possibly text documents) when configuration changes
@snowsignal snowsignal added bug Something isn't working server Related to the LSP server labels May 22, 2024
Copy link
Contributor

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

Copy link
Member

@dhruvmanila dhruvmanila left a comment

Choose a reason for hiding this comment

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

Can you update the test plan to include a video showcasing this fix? I think that would helpful to quickly review this.

@snowsignal snowsignal merged commit 573facd into main May 22, 2024
19 checks passed
@snowsignal snowsignal deleted the jane/server/fix-config-reload branch May 22, 2024 18:20
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.

2 participants