refactor: Split CliSettings from Settings #1891

not-my-profile · 2023-01-15T10:11:39Z

No description provided.

We want to automatically derive Hash for the library settings, which requires us to split off all the settings unused by the library (since these shouldn't affect the hash used by ruff_cli::cache).

charliermarsh · 2023-01-15T18:55:24Z

src/settings/mod.rs

@@ -12,6 +12,7 @@ use globset::{Glob, GlobMatcher, GlobSet};
 use itertools::Either::{Left, Right};
 use itertools::Itertools;
 use once_cell::sync::Lazy;
+#[cfg(test)]


Is this intended?

Yes the import is only used in the test-only for_rule(s) functions and omitting this attribute results in clippy complaining about an unused import. An alternative would be to move the import into the functions but I don't really see the benefit.

charliermarsh · 2023-01-15T18:56:21Z

src/settings/mod.rs

+    pub fn from_configuration(config: Configuration, project_root: &Path) -> Result<Self> {
+        Ok(Self {
+            cli: CliSettings {
+                cache_dir: config


Should we impl From<&Configuration>?

We'd have to get rid of the additional project_root parameter ... I am not sure if that's desirable.

charliermarsh · 2023-01-15T18:57:21Z

src/settings/mod.rs

+#[derive(Debug)]
+pub struct AllSettings {
+    pub cli: CliSettings,
+    pub lib: Settings,


Idly wondering if this should be LibSettings, and AllSettings should be Settings.

Eh, AllSettings or CombinedSettings is probably for the aggregate struct. Although I do wonder if Settings should be LibSettings or something.

I don't think CliSettings belongs in the ruff crate at all ... my plan is to move it to ruff_cli in the future. So I think it makes sense to keep it as Settings since these ought to be the only settings in the library crate.

If it turns out that we cannot move CliSettings I am alright with changing Settings to LibSettings but I'd like to try that factorisation first (at some point in the future after this PR has been merged).

not-my-profile added 2 commits January 15, 2023 10:12

Mark Settings::for_rule(s) as test-only

5c03064

refactor: Split CliSettings from Settings

d1fbbbd

We want to automatically derive Hash for the library settings, which requires us to split off all the settings unused by the library (since these shouldn't affect the hash used by ruff_cli::cache).

not-my-profile mentioned this pull request Jan 15, 2023

Derive Hash instead of implementing it by hand #1890

Merged

charliermarsh reviewed Jan 15, 2023

View reviewed changes

charliermarsh merged commit d75d6d7 into astral-sh:main Jan 15, 2023

MichaReiser mentioned this pull request Sep 20, 2023

Unify Settings and AllSettings #7532

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: Split CliSettings from Settings #1891

refactor: Split CliSettings from Settings #1891

not-my-profile commented Jan 15, 2023

charliermarsh Jan 15, 2023

not-my-profile Jan 15, 2023

charliermarsh Jan 15, 2023

not-my-profile Jan 15, 2023

charliermarsh Jan 15, 2023

charliermarsh Jan 15, 2023

not-my-profile Jan 15, 2023 •

edited

Loading

refactor: Split CliSettings from Settings #1891

refactor: Split CliSettings from Settings #1891

Conversation

not-my-profile commented Jan 15, 2023

charliermarsh Jan 15, 2023

Choose a reason for hiding this comment

not-my-profile Jan 15, 2023

Choose a reason for hiding this comment

charliermarsh Jan 15, 2023

Choose a reason for hiding this comment

not-my-profile Jan 15, 2023

Choose a reason for hiding this comment

charliermarsh Jan 15, 2023

Choose a reason for hiding this comment

charliermarsh Jan 15, 2023

Choose a reason for hiding this comment

not-my-profile Jan 15, 2023 • edited Loading

Choose a reason for hiding this comment

not-my-profile Jan 15, 2023 •

edited

Loading