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.

Sign up for GitHub

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

Jump to bottom

Consider nested configs for settings reloading #12253

Merged
merged 1 commit into from
Jul 12, 2024
Merged

Consider nested configs for settings reloading #12253

merged 1 commit into from
Jul 12, 2024

Conversation

dhruvmanila
Copy link
Member

@dhruvmanila dhruvmanila commented Jul 9, 2024 •
edited
Loading

Summary

This PR fixes a bug in the settings reloading logic to consider nested configuration in a workspace.

fixes: #11766

Test Plan

Screen.Recording.2024-07-09.at.13.00.11.mov

@dhruvmanila dhruvmanila force-pushed the dhruv/reload-hierarchical-config branch from c722ae2 to eeee047 Compare July 9, 2024 07:13
@dhruvmanila dhruvmanila mentioned this pull request Jul 9, 2024
Merged
@dhruvmanila dhruvmanila added the bug Something isn't working label Jul 9, 2024
@dhruvmanila dhruvmanila force-pushed the dhruv/reload-hierarchical-config branch from eeee047 to d536841 Compare July 9, 2024 07:32
@dhruvmanila dhruvmanila added the server Related to the LSP server label Jul 9, 2024
@dhruvmanila dhruvmanila marked this pull request as ready for review July 9, 2024 07:32
@dhruvmanila dhruvmanila mentioned this pull request Jul 9, 2024
Closed
dhruvmanila
dhruvmanila commented Jul 9, 2024
View reviewed changes
crates/ruff_server/src/session/index.rs Outdated
Comment on lines 315 to 317
for (root, settings) in self.settings.range_mut(enclosing_folder.to_path_buf()..) {
if !root.starts_with(enclosing_folder) {
for (root, settings) in &mut self.settings {
if !enclosing_folder.starts_with(root) {
Copy link
Member Author

Choose a reason for hiding this comment

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

There are two main changes:

  1. We loop over every workspace folder. The range_mut is incorrect here because the "enclosing_folder" would be within the workspace.
  2. The starts_with call is incorrect. We should be checking if the folder is part of the workspace or not and not the other way around which will always be incorrect for nested folders.

Copy link
Member

Choose a reason for hiding this comment

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

I wonder if this is correct though: Let's say we have workspace a and b and we add a configuration to b/foo so that root = a and enclosing_folder=b/foo. We would immediately hit the break here.

I think we want self.settings.range(..enclosing_folder).rev() and we take the paths for as long as they start with enclosing_folder.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, that's correct. Thanks. Let me update it.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is actually not correct either because the root folder can never start with an enclosing_folder. This is the main reason I switched the starts_with call in (2). Let me think about this a bit more considering your example as well.

@dhruvmanila dhruvmanila linked an issue Jul 9, 2024 that may be closed by this pull request
ruff server does not load new configuration files when they are created #11766
Closed
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Jul 9, 2024 •
edited
Loading

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/server-include branch 2 times, most recently from 8af9e89 to 3b3cf58 Compare July 9, 2024 12:35
@dhruvmanila dhruvmanila force-pushed the dhruv/reload-hierarchical-config branch from d536841 to 9b4fe92 Compare July 9, 2024 12:38
@dhruvmanila dhruvmanila force-pushed the dhruv/reload-hierarchical-config branch from 9b4fe92 to 1f8b9a1 Compare July 9, 2024 13:21
MichaReiser
MichaReiser approved these changes Jul 9, 2024
View reviewed changes
crates/ruff_server/src/session/index.rs Outdated
if !root.starts_with(enclosing_folder) {
break;
continue;
Copy link
Member

Choose a reason for hiding this comment

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

Is break still incorrect here? I get the impression that we could use break here but not entirely sure

Base automatically changed from dhruv/server-include to main July 10, 2024 04:12
dhruvmanila added a commit that referenced this pull request Jul 10, 2024
@dhruvmanila
Conside include, extend-include for the native server (#12252)
0bb2fc6 
## Summary

This PR updates the native server to consider the `include` and
`extend-include` file resolver settings.

fixes: #12242 

## Test Plan

Note: Settings reloading doesn't work for nested configs which is fixed
in #12253 so the preview here only showcases root level config.

https://github.com/astral-sh/ruff/assets/67177269/e8969128-c175-4f98-8114-0d692b906cc8
@dhruvmanila dhruvmanila force-pushed the dhruv/reload-hierarchical-config branch 2 times, most recently from 3d73ff8 to 8b72531 Compare July 10, 2024 08:53
MichaReiser
MichaReiser approved these changes Jul 10, 2024
View reviewed changes
Copy link
Member

@MichaReiser MichaReiser left a comment

Choose a reason for hiding this comment

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

To me it's still unclear if break would be sufficient instead of continue but I'm not a 100% sure.

@dhruvmanila
Copy link
Member Author

To me it's still unclear if break would be sufficient instead of continue but I'm not a 100% sure.

I thought about this more and I think break should be sufficient. The only place where continue would matter is if there are at least three workspaces with the following characteristics:

  1. Should be refreshed
  2. Should be continue
  3. Config file under this workspace has been changed
    The output of range operation should yield [3, 2, 1]. I can't come up with a workspace layout where this matches. The usage of BtreeMap avoids this kinds of cases where continue would be required.

@dhruvmanila dhruvmanila force-pushed the dhruv/reload-hierarchical-config branch from 8b72531 to 6c0aa51 Compare July 12, 2024 04:56
@dhruvmanila dhruvmanila enabled auto-merge (squash) July 12, 2024 04:57
@dhruvmanila dhruvmanila merged commit 90e9aae into main Jul 12, 2024
17 checks passed
@dhruvmanila dhruvmanila deleted the dhruv/reload-hierarchical-config branch July 12, 2024 05:00
@BrewTestBot BrewTestBot mentioned this pull request Jul 15, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MichaReiser MichaReiser MichaReiser approved these changes

Assignees
No one assigned
Labels
bug Something isn't working server Related to the LSP server
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

ruff server does not load new configuration files when they are created
2 participants
@dhruvmanila @MichaReiser