Respect excludes in ruff server configuration discovery
#11551
Conversation
charliermarsh
added
bug
| continue; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
This is loosely drawn from python_files_in_path.
charliermarsh
force-pushed
the
charlie/extension-excludes
branch
from
a951ab5 to
e9cfb77
Compare
|
charliermarsh
force-pushed
the
charlie/extension-excludes
branch
from
e9cfb77 to
2425af0
Compare
| @@ -80,25 +85,23 @@ impl RuffSettings { | |||
|
|
|||
| impl RuffSettingsIndex { | |||
| pub(super) fn new(root: &Path, editor_settings: &ResolvedEditorSettings) -> Self { | |||
| let mut index = BTreeMap::default(); | |||
| let index = RefCell::new(BTreeMap::new()); | |||
There was a problem hiding this comment.
We need some sort of mechanism here because we have to both immutably borrow index in filter_entry (which has to be a closure) and mutably borrow it in the for loop body.
There was a problem hiding this comment.
Yeah, I don't see any obvious way to avoid using RefCell without collecting them all in a vector.
There was a problem hiding this comment.
So, if I understand this change correctly, does it mean that the root cause of the issue is that the server uses a low level API from the linter (check_path) which means the server doesn't benefit from any logic before that?
If that's the case, then I don't think the server benefits from the cache either and I wonder what other logic would be required here. Nevertheless, I don't think that needs to be checked or addressed in this PR.
| .rev() | ||
| .find(|(path, _)| directory.starts_with(path)) |
There was a problem hiding this comment.
Nit
| .rev() | |
| .find(|(path, _)| directory.starts_with(path)) | |
| .rfind(|(path, _)| directory.starts_with(path)) |
| { | ||
| if let Some(file_name) = directory.file_name() { | ||
| let candidate = Candidate::new(&directory); | ||
| let basename = Candidate::new(file_name); | ||
| if match_candidate_exclusion( | ||
| &candidate, | ||
| &basename, | ||
| &settings.file_resolver.exclude, | ||
| ) { | ||
| tracing::debug!("Ignored path via `exclude`: {}", directory.display()); | ||
| return false; | ||
| } else if match_candidate_exclusion( | ||
| &candidate, | ||
| &basename, | ||
| &settings.file_resolver.extend_exclude, | ||
| ) { | ||
| tracing::debug!( | ||
| "Ignored path via `extend-exclude`: {}", | ||
| directory.display() | ||
| ); | ||
| return false; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| true | ||
| }) |
There was a problem hiding this comment.
I first assumed that the { starts a new block, and only after removing it realised that it starts the for body.
I think it would be good to extract the WalkDir::new into a variable
|
A possible alternative to avoid the |
Yes, I believe so. |
|
If either of you want to modify and merge + release this today, feel free -- otherwise I may be able to get to it tonight. |
|
Gonna try and fix-up + merge this now. |
charliermarsh
force-pushed
the
charlie/extension-excludes
branch
2 times, most recently
from
15e168e to
6b6adfb
Compare
|
I removed |
charliermarsh
force-pushed
the
charlie/extension-excludes
branch
from
6b6adfb to
60464d3
Compare
Summary
Right now, we're discovering configuration files even within (e.g.) virtual environments, because we're recursing without respecting the
excludefield on parent configuration.Closes astral-sh/ruff-vscode#478.
Test Plan
Installed Pandas; verified that I saw no warnings: