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

Introduce LinterSettings #7543

Merged
merged 1 commit into from Sep 20, 2023
Merged

Introduce LinterSettings #7543

merged 1 commit into from Sep 20, 2023

Conversation

MichaReiser
Copy link
Member

@MichaReiser MichaReiser commented Sep 20, 2023

Stack Summary

This stack splits Settings into FormatterSettings and LinterSettings and moves it into ruff_workspace. This change is necessary to add the FormatterSettings to Settings without adding ruff_python_formatter as a dependency to ruff_linter (and the linter should not contain the formatter settings).

A quick overview of our settings struct at play:

  • Options: 1:1 representation of the options in the pyproject.toml or ruff.toml. Used for deserialization.
  • Configuration: Resolved Options, potentially merged from multiple configurations (when using extend). The representation is very close if not identical to the Options.
  • Settings: The resolved configuration that uses a data format optimized for reading. Optional fields are initialized with their default values. Initialized by Configuration::into_settings .

The goal of this stack is to split Settings into tool-specific resolved Settings that are independent of each other. This comes at the advantage that the individual crates don't need to know anything about the other tools. The downside is that information gets duplicated between Settings. Right now the duplication is minimal (line-length, tab-width) but we may need to come up with a solution if more expensive data needs sharing.

This stack focuses on Settings. Splitting Configuration into some smaller structs is something I'll follow up on later.

PR Summary

This PR extracts the linter-specific settings into a new LinterSettings struct and adds it as a linter field to the Settings struct. This is in preparation for moving Settings from ruff_linter to ruff_workspace

Test Plan

cargo test

pub show_source: bool,

pub resolver: ResolverSettings,
pub struct LinterSettings {
Copy link
Member Author

Choose a reason for hiding this comment

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

The changes in this file are the main changes. Everything else are just downstream refactors to use the new structure.

ignore_init_module_imports: self.ignore_init_module_imports.unwrap_or_default(),
line_length: self.line_length.unwrap_or_default(),
tab_size: self.tab_size.unwrap_or_default(),
namespace_packages: self.namespace_packages.unwrap_or_default(),
Copy link
Member

Choose a reason for hiding this comment

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

I feel like some of these aren't linter-specific, like namespace_packages and src -- they would be generally applicable to any tool that does module / file resolution. But it's not important for now.

Copy link
Member Author

Choose a reason for hiding this comment

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

That's true, but the Settings are the resolved view for a tool. My current, not so nice approach, is to duplicate settings that apply to multiple tools when resolving the Configuration. Meaning, the Configuration applies` the right inheritance rules and, as a result, may end up initializing common fields identically.

I don't think this is a performance problem because we only do this once per setting and settings tend to not have very large structures.

.map(PyUpgradeOptions::into_settings)
.unwrap_or_default(),
linter: LinterSettings {
target_version,
Copy link
Member

Choose a reason for hiding this comment

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

Should this be lifted up?

Copy link
Member Author

Choose a reason for hiding this comment

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

The linter needs access to target_version. We can't access it if we lift it up. I'll lift it up in the Configuration, but the each Setting object needs its own copy of the target_version.

@codspeed-hq
Copy link

codspeed-hq bot commented Sep 20, 2023

CodSpeed Performance Report

Merging #7543 will improve performances by 2%

Comparing linter-settings (308bae8) with main (8f41eab)

Summary

⚡ 1 improvements
✅ 24 untouched benchmarks

Benchmarks breakdown

Benchmark main linter-settings Change
linter/default-rules[pydantic/types.py] 38.8 ms 38 ms +2%

@MichaReiser MichaReiser changed the title Introduce LinterSettings and ResolverSettings Introduce LinterSettings Sep 20, 2023
@MichaReiser MichaReiser force-pushed the linter-settings branch 2 times, most recently from 4cdd24c to 0efdc80 Compare September 20, 2023 14:36
Base automatically changed from resolver-settings to main September 20, 2023 14:37
@MichaReiser
Copy link
Member Author

MichaReiser commented Sep 20, 2023

Merge Activity

  • Sep 20, 10:41 AM: Graphite rebased this pull request after merging its parent, because this pull request is set to merge when ready.
  • Sep 20, 11:02 AM: @MichaReiser merged this pull request with Graphite.

@github-actions
Copy link
Contributor

PR Check Results

Ecosystem

✅ ecosystem check detected no changes.

@MichaReiser MichaReiser merged commit b34278e into main Sep 20, 2023
16 checks passed
@MichaReiser MichaReiser deleted the linter-settings branch September 20, 2023 15:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
internal An internal refactor or improvement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants