Consider nested configs for settings reloading#12253
Conversation
d83b0f2 to
9041186
Compare
c722ae2 to
eeee047
Compare
eeee047 to
d536841
Compare
| 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) { |
There was a problem hiding this comment.
There are two main changes:
- We loop over every workspace folder. The
range_mutis incorrect here because the "enclosing_folder" would be within the workspace. - The
starts_withcall 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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Oh, that's correct. Thanks. Let me update it.
There was a problem hiding this comment.
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.
|
8af9e89 to
3b3cf58
Compare
d536841 to
9b4fe92
Compare
3b3cf58 to
83003dc
Compare
9b4fe92 to
1f8b9a1
Compare
| { | ||
| if !root.starts_with(enclosing_folder) { | ||
| break; | ||
| continue; |
There was a problem hiding this comment.
Is break still incorrect here? I get the impression that we could use break here but not entirely sure
## 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
3d73ff8 to
8b72531
Compare
MichaReiser
left a comment
There was a problem hiding this comment.
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
|
8b72531 to
6c0aa51
Compare
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