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

Use fallback settings when indexing the project #12362

Merged
merged 2 commits into from
Jul 18, 2024
Merged

Conversation

dhruvmanila
Copy link
Member

@dhruvmanila dhruvmanila commented Jul 17, 2024

Summary

This PR updates the settings index building logic in the language server to consider the fallback settings for applying ignore filters in WalkBuilder and the exclusion via exclude / extend-exclude.

This flow matches the one in the ruff CLI where the root settings is built by (1) finding the workspace setting in the ancestor directory (2) finding the user configuration if that's missing and (3) fallback to using the default configuration.

Previously, the index building logic was being executed before (2) and (3). This PR reverses the logic so that the exclusion / respect_gitignore is being considered from the default settings if there's no workspace / user settings. This has the benefit that the server no longer enters the .git directory or any other excluded directory when a user opens a file in the home directory.

Related to #11366

Test plan

Opened a test file from the home directory and confirmed with the debug trace (removed in #12360) that the server excludes the .git directory when indexing.

@dhruvmanila dhruvmanila added the server Related to the LSP server label Jul 17, 2024
Copy link
Contributor

github-actions bot commented Jul 17, 2024

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

);
return WalkState::Continue;
}
.map(|(_, settings)| settings.clone())
Copy link
Member

Choose a reason for hiding this comment

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

Are these clones necessary?

Copy link
Member

Choose a reason for hiding this comment

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

I think these are all Arc<RuffSettings> so this should be cheap.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, these are all Arcs

Copy link
Member Author

Choose a reason for hiding this comment

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

It otherwise throws "rustc: temporary value dropped while borrowed" if I use a reference

                    let settings = index
                        .read()
                        .unwrap()
                        .range(..directory.clone())
                        .rfind(|(path, _)| directory.starts_with(path))
                        .map(|(_, settings)| settings)
                        .unwrap_or(&fallback);

);
return WalkState::Continue;
}
.map(|(_, settings)| settings.clone())
Copy link
Member

Choose a reason for hiding this comment

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

I think these are all Arc<RuffSettings> so this should be cheap.

crates/ruff_server/src/session/index/ruff_settings.rs Outdated Show resolved Hide resolved
@dhruvmanila dhruvmanila merged commit ebe5b06 into main Jul 18, 2024
20 checks passed
@dhruvmanila dhruvmanila deleted the dhruv/fallback branch July 18, 2024 03:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
server Related to the LSP server
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants