Refactor settings #1930

not-my-profile · 2023-01-17T09:48:58Z

No description provided.

charliermarsh · 2023-01-17T12:31:32Z

src/settings/mod.rs

+        HashableGlobMatcher,
+        HashableHashSet<RuleCode>,
+    )>,
+


Is this an intentional newline?

Yes I inserted it as a conceptual break between settings related to rule selection and other core settings (which are different from resolver or rule-specific settings).

charliermarsh · 2023-01-17T12:32:03Z

src/settings/mod.rs

+        )) {
+            let fix = fixable.contains(&code);
+            rules.enable(code, fix);
+        }


Can we move this into its own utility function, to keep the constructor declarative?

We could however I don't really see the point since 1) that function won't be used anywhere else and 2) the function signature would have to be quite complex since it would need to take config.fixable, config.unfixable, config.select and config.ignore (we cannot just pass &Configuration because we want to avoid needless clones() and so we'd need to have 4 owned parameters for the borrow checker).

I think the point would be to increase the readability of this constructor :)

Fair enough, done :)

Thank you, will merge shortly :)

charliermarsh · 2023-01-17T12:32:46Z

src/settings/rule_table.rs

+
+    /// Returns whether the given rule should be checked.
+    pub fn enabled(&self, code: &RuleCode) -> bool {
+        self.enabled.contains_key(code)


Do you still plan on changing this to use macro-based lookups or no longer viable?

No I don't think macro-based lookups make sense (especially considering that they don't give any substantial performance gain). I think we should strive to decouple the ruff core from our linter implementations and requiring one boolean field in a struct for every rule is very much the opposite of decoupling.

charliermarsh · 2023-01-17T12:33:32Z

src/settings/rule_table.rs

+
+    /// Returns an iterator over all enabled rules.
+    pub fn iter_enabled(&self) -> hash_map::Keys<RuleCode, bool> {
+        self.enabled.keys()


Could we just implement iter here?

Yes we could but I think it would be less explicit ... iterate over what? All rules? All enabled rules? All rules that have autofix enabled? It's not unthinkable that we at some point end up having a need to also iterate over rules that have autofix enabled.

Eh, I think it's reasonable to assume that iterating over a RuleTable iterates over the enabled or active rules. But, I'm not gonna push it.

I agree that this is a reasonable assumption however I think IntoIterator implementations ideally don't require any assumptions.

Fair enough!

charliermarsh · 2023-01-17T13:05:29Z

src/settings/mod.rs

-            pydocstyle: pydocstyle::settings::Settings::default(),
-            pyupgrade: pyupgrade::settings::Settings::default(),
+            rules: [rule_code].into(),
+            ..Settings::default()


The Settings struct previously contained the fields: pub enabled: HashableHashSet<RuleCode>, pub fixable: HashableHashSet<RuleCode>, This commit merges both fields into one by introducing a new RuleTable type, wrapping HashableHashMap<RuleCode, bool>, which has the following benefits: 1. It makes the invalid state that a rule is disabled but fixable unrepresentable. 2. It encapsulates the implementation details of the table. (It currently uses an FxHashMap but that may change.) 3. It results in more readable code. settings.rules.enabled(rule) settings.rules.should_fix(rule) is more readable than: settings.enabled.contains(rule) settings.fixable.contains(rule)

[![Mend Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com) This PR contains the following updates: | Package | Change | Age | Adoption | Passing | Confidence | |---|---|---|---|---|---| | [ruff](https://togithub.com/charliermarsh/ruff) | `^0.0.224` -> `^0.0.225` | [![age](https://badges.renovateapi.com/packages/pypi/ruff/0.0.225/age-slim)](https://docs.renovatebot.com/merge-confidence/) | [![adoption](https://badges.renovateapi.com/packages/pypi/ruff/0.0.225/adoption-slim)](https://docs.renovatebot.com/merge-confidence/) | [![passing](https://badges.renovateapi.com/packages/pypi/ruff/0.0.225/compatibility-slim/0.0.224)](https://docs.renovatebot.com/merge-confidence/) | [![confidence](https://badges.renovateapi.com/packages/pypi/ruff/0.0.225/confidence-slim/0.0.224)](https://docs.renovatebot.com/merge-confidence/) | --- ### Release Notes <details> <summary>charliermarsh/ruff</summary> ### [`v0.0.225`](https://togithub.com/charliermarsh/ruff/releases/tag/v0.0.225) [Compare Source](https://togithub.com/charliermarsh/ruff/compare/v0.0.224...v0.0.225) #### What's Changed - Define origin names & URLs within doc comments by [@not-my-profile](https://togithub.com/not-my-profile) in [astral-sh/ruff#1929 - Refactor settings by [@not-my-profile](https://togithub.com/not-my-profile) in [astral-sh/ruff#1930 - Allow duplicate enum values for enum.auto() by [@charliermarsh](https://togithub.com/charliermarsh) in [astral-sh/ruff#1933 - Move `@functools.cache` rewrites to their own rule by [@charliermarsh](https://togithub.com/charliermarsh) in [astral-sh/ruff#1938 - cli: Catch panics to tell the user to report them by [@not-my-profile](https://togithub.com/not-my-profile) in [astral-sh/ruff#1928 - Avoid autofixing comma rules when --fix is not set by [@charliermarsh](https://togithub.com/charliermarsh) in [astral-sh/ruff#1940 - Avoid broken autofix for `SIM103` with `elif` by [@charliermarsh](https://togithub.com/charliermarsh) in [astral-sh/ruff#1944 - Implement `flake8-no-pep420` by [@edgarrmondragon](https://togithub.com/edgarrmondragon) in [astral-sh/ruff#1942 - Exempt `contextlib.ExitStack()` for SIM115 rules by [@charliermarsh](https://togithub.com/charliermarsh) in [astral-sh/ruff#1946 - Restrict SIM105 to try blocks with a body of one simple statement by [@andersk](https://togithub.com/andersk) in [astral-sh/ruff#1948 **Full Changelog**: astral-sh/ruff@v0.0.224...v0.0.225 </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Enabled. ♻ **Rebasing**: Whenever PR is behind base branch, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] If you want to rebase/retry this PR, check this box --- This PR has been generated by [Mend Renovate](https://www.mend.io/free-developer-tools/renovate/). View repository job log [here](https://app.renovatebot.com/dashboard#github/ixm-one/pytest-cmake-presets).  Signed-off-by: Renovate Bot <bot@renovateapp.com> Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

refactor: Group Settings fields

80368c4

not-my-profile force-pushed the refactor-settings branch 5 times, most recently from e524724 to 255582a Compare January 17, 2023 10:27

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

Use boolean fields instead of HashMap lookups for enabled checks #1896

Closed

charliermarsh reviewed Jan 17, 2023

View reviewed changes

not-my-profile force-pushed the refactor-settings branch from 255582a to be6dc5d Compare January 17, 2023 13:15

not-my-profile added 4 commits January 17, 2023 14:21

refactor: Make resolve_codes take IntoIterator instead of Iterator

d982336

refactor: Implement Default for Settings

35ffd9a

refactor: Declare defaults once in settings::defaults

d6ffa0e

not-my-profile force-pushed the refactor-settings branch from be6dc5d to d6ffa0e Compare January 17, 2023 13:21

charliermarsh merged commit 30e133f into astral-sh:main Jan 17, 2023

renovate bot mentioned this pull request Feb 4, 2023

chore(deps): update dependency ruff to ^0.0.247 ibis-project/ibis-substrait#478

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor settings #1930

Refactor settings #1930

not-my-profile commented Jan 17, 2023

charliermarsh Jan 17, 2023

not-my-profile Jan 17, 2023 •

edited

charliermarsh Jan 17, 2023

not-my-profile Jan 17, 2023

charliermarsh Jan 17, 2023

not-my-profile Jan 17, 2023

charliermarsh Jan 17, 2023

charliermarsh Jan 17, 2023

not-my-profile Jan 17, 2023

charliermarsh Jan 17, 2023

not-my-profile Jan 17, 2023

charliermarsh Jan 17, 2023

not-my-profile Jan 17, 2023

charliermarsh Jan 17, 2023

charliermarsh Jan 17, 2023

Refactor settings #1930

Refactor settings #1930

Conversation

not-my-profile commented Jan 17, 2023

Choose a reason for hiding this comment

not-my-profile Jan 17, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

not-my-profile Jan 17, 2023 •

edited