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

Fallback to requires-python in certain cases when target-version is not found #16319

Merged
merged 36 commits into from
Mar 13, 2025
Merged

Fallback to requires-python in certain cases when target-version is not found #16319

Changes from 1 commit
Commits
Show all changes
36 commits
Select commit Hold shift + click to select a range
7bbda72
test fixtures
dylwil3 Feb 22, 2025
186235a
ruff.toml fallback to requires-python in same directory
dylwil3 Feb 22, 2025
92bc35a
no config fallback to ancestor requires-python
dylwil3 Feb 22, 2025
7c2c9b1
clippy
dylwil3 Feb 22, 2025
812fb20
update snapshot
dylwil3 Feb 22, 2025
759c0ba
try to appease windows
dylwil3 Feb 22, 2025
cc7d0bb
add TargetVersionStrategy to toggle fallback to requires_python
dylwil3 Feb 23, 2025
c8c10cd
add ConfigurationProvenance to switch resolution behavior
dylwil3 Feb 23, 2025
3c3c812
clippy
dylwil3 Feb 23, 2025
9fbe52a
debug log when parsing fails during fallback discovery
dylwil3 Feb 23, 2025
4a2d9e1
change name to ConfigurationOrigin
dylwil3 Feb 28, 2025
cd37ed8
make ConfigurationOrigin Copy
dylwil3 Feb 28, 2025
7f4403f
use Unknown ConfigurationOrigin instead of None
dylwil3 Feb 28, 2025
3613121
TargetVersionStrategy Standard --> UseDefault
dylwil3 Feb 28, 2025
1b52717
derive Relativity from ConfigurationOrigin
dylwil3 Feb 28, 2025
e2b30e9
nit: move test attr to above fn
dylwil3 Feb 28, 2025
9039594
nit: deriving --> derived
dylwil3 Feb 28, 2025
7e1f777
nit: use From instead of Into
dylwil3 Feb 28, 2025
0d99dba
nit: spacing
dylwil3 Feb 28, 2025
081518a
nit: shadow with as_ref
dylwil3 Feb 28, 2025
3c45426
clarify logic for default settings fallback
dylwil3 Feb 28, 2025
2a20b69
add test for shared config
dylwil3 Feb 28, 2025
123006f
update snapshots
dylwil3 Feb 28, 2025
513b1c8
display derived version in log
dylwil3 Mar 4, 2025
5f3ec8c
update snapshots
dylwil3 Mar 4, 2025
2d22b76
test --show-settings and add test with tool table
dylwil3 Mar 4, 2025
511129e
windows strikes again
dylwil3 Mar 4, 2025
ac55239
test check on directory instead of single file
dylwil3 Mar 12, 2025
54acafa
config origin knwon to be Ancestor in resolver - pass test
dylwil3 Mar 12, 2025
a1a7548
update snapshots from rebase
dylwil3 Mar 12, 2025
b098255
one more missed snapshot
dylwil3 Mar 12, 2025
229e905
test cli override
dylwil3 Mar 12, 2025
6093d74
for server use editor config with fallback version as fallback
dylwil3 Mar 12, 2025
f5d0203
Collect logs from the `log` crate in the server logs
MichaReiser Mar 13, 2025
49a593e
Use the `requires-python` fallback with a user-level configuration
MichaReiser Mar 13, 2025
dc26d44
Align server indexing with CLI behavior
MichaReiser Mar 13, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 12 additions & 2 deletions crates/ruff_server/src/session/index/ruff_settings.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,11 @@ use std::sync::Arc;

use anyhow::Context;
use ignore::{WalkBuilder, WalkState};
use ruff_python_ast::PythonVersion;

use ruff_linter::settings::types::GlobPath;
use ruff_linter::{settings::types::FilePattern, settings::types::PreviewMode};
use ruff_workspace::pyproject::find_fallback_target_version;
use ruff_workspace::resolver::match_exclusion;
use ruff_workspace::Settings;
use ruff_workspace::{
Expand Down Expand Up @@ -83,8 +85,13 @@ impl RuffSettings {
/// Constructs [`RuffSettings`] by merging the editor-defined settings with the
/// default configuration.
fn editor_only(editor_settings: &ResolvedEditorSettings, root: &Path) -> RuffSettings {
let mut config = Configuration::default();
if let Some(fallback_version) = find_fallback_target_version(root) {
config.target_version = Some(PythonVersion::from(fallback_version));
};

Copy link
Member

Choose a reason for hiding this comment

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

Does that mean that we'll pick up the requires-python constraint even if a user uses editor-only in their settings. I'd be a bit confused if editor-only loads any project settings. @dhruvmanila what's your take on this?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I don't think we should be looking at any of the config files if the configuration preference is editorOnly unless the user has explicitly asks us using the ruff.configuration.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think we won't have gotten this far in the code in that case, though, right? We exited earlier here:

ruff/crates/ruff_server/src/session/index/ruff_settings.rs

Lines 114 to 123 in dd2313a

if editor_settings.configuration_preference == ConfigurationPreference::EditorOnly {
tracing::debug!(
"Using editor-only settings for workspace: {} (skipped indexing)",
root.display()
);
return RuffSettingsIndex {
index: BTreeMap::default(),
fallback: Arc::new(RuffSettings::editor_only(editor_settings, root)),
};
}

Copy link
Member

Choose a reason for hiding this comment

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

That branch will be calling RuffSettings::editor_only to create a default configuration so we'll be getting there in this case.

let settings = EditorConfigurationTransformer(editor_settings, root)
.transform(Configuration::default())
.transform(config)
.into_settings(root)
.expect("editor configuration should merge successfully with default configuration");

Expand Down Expand Up @@ -292,7 +299,10 @@ impl RuffSettingsIndex {
}
}
}
Ok(None) => {}
Ok(None) => {
let settings = RuffSettings::editor_only(editor_settings, &directory);
index.write().unwrap().insert(directory, Arc::new(settings));
}
Copy link
Member

Choose a reason for hiding this comment

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

If I'm understanding this change correctly, we'd now have a fallback settings for every directory in the project. I think this might create an unexpected behavior because for a file in a nested directory, it would use this fallback settings instead of the settings from the project root? Let me test this out.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I tested out, I think this is an incorrect fix. Consider the following directory structure:

.
├── nested
│   └── foo
│       └── test.py
├── pyproject.toml
└── test.py

The nested/foo/test.py does not consider any settings from pyproject.toml, it's only the test.py file that will consider it.

Copy link
Member

Choose a reason for hiding this comment

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

It's not entirely clear to me why the change here is necessary. Shouldn't the changes above be sufficient?

Err(err) => {
tracing::error!("{err:#}");
has_error.store(true, Ordering::Relaxed);
Expand Down
Loading