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

Add [format|lint].exclude options #8000

Merged
merged 1 commit into from Oct 18, 2023
Merged

Conversation

MichaReiser
Copy link
Member

@MichaReiser MichaReiser commented Oct 17, 2023

Summary

This PR introduces the new options lint.exclude and format.exclude that allow excluding files from linting or formatting only.

Being able to exclude entire files is useful when:

  • you have generated code that you only want to format but not lint (or the other way round)
  • you have sub modules that you only want to lint because the code isn't using a formatter yet (but the rest of the project is)

Closes #7645

Considerations

The check command (and now the format command) exposes the exclude option. This is the global exclude and not the lint.exclude or format.exclude. Only exposing the global exclude option should be sufficient and make little difference except if the user wants to clear out the lint|format.exclude option via the CLI.

Only exposing the global exclude option makes sense for me, considering that we plan to unify linting and formatting under the check command in the future. Whether this is the right design for format is less clear to me but I think it's in line with check and we can still decide to add a format-exclude CLI option if it proves necessary.

Test Plan

I added new integration tests that test the tool specific exclusion.

I played around with the airflow repository and excluded files for linting or formatting only and verified that they are only excluded for the specific tool (and the result is the same as when excluding the files globally). Verified that a file passed directly gets linted/formated regardless if it is excluded or not.

@@ -22,6 +22,15 @@ pub(crate) fn check_stdin(
if !python_file_at_path(filename, pyproject_config, overrides)? {
return Ok(Diagnostics::default());
}

let lint_settings = &pyproject_config.settings.linter;
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's a bit annoying that we have to duplicate this check everywhere. It would be nice to have a single CLI logic that handles the traversal of all files and decides if a file is eligible for a specific tool... Another day

@@ -377,7 +377,7 @@ pub struct Options {

/// The lint sections specified at the top level.
#[serde(flatten)]
pub lint_top_level: LintOptions,
pub lint_top_level: LintCommonOptions,
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 LintCommonOptions is necessary because we don't want to flatten the exclude option into the top-level settings (would conflict with the global exclude setting)

Copy link
Member

Choose a reason for hiding this comment

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

Can this be removed when we remove the top-level lint options support?

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 that's the plan!

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 we should probably wait a while to drop support for top-level lint options though, maybe 0.3? maybe later? We should open a tracking issue we can add todos to

Copy link
Member Author

Choose a reason for hiding this comment

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

Can this be removed when we remove the top-level lint options support?
Yes

I think we should probably wait a while to drop support for top-level lint options though, maybe 0.3? maybe later? We should open a tracking issue we can add todos to

I don't know the timeline yet but my thinking is:

  • Not before we ship the stable formatter (the main goal of the beta is to get feedback on the configuration)
  • Promote the lint section over the formatter section (by changing the documentation)
  • Deprecate the top-level settings
  • Eventually, remove support for top-level lint settings

I expect this to be 0.3+, maybe even 1.0

@@ -276,7 +278,7 @@ pub fn python_files_in_path(
paths: &[PathBuf],
pyproject_config: &PyprojectConfig,
transformer: &dyn ConfigurationTransformer,
) -> Result<(Vec<Result<DirEntry, ignore::Error>>, Resolver)> {
) -> Result<(Vec<Result<ResolvedFile, ignore::Error>>, Resolver)> {
Copy link
Member Author

Choose a reason for hiding this comment

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

I had to introduce a new ResolvedFile type to know whether the file was explicitly passed to the CLI or not. I could have used DirEntry::depth but I found that it leaks internal details about python_files_in_path

@MichaReiser MichaReiser force-pushed the format-lint-exclude-option branch 2 times, most recently from dafec38 to e7ec415 Compare October 17, 2023 03:28
@MichaReiser MichaReiser added formatter Related to the formatter configuration Related to settings and configuration labels Oct 17, 2023
@github-actions
Copy link

github-actions bot commented Oct 17, 2023

PR Check Results

Ecosystem

✅ ecosystem check detected no changes.

@MichaReiser MichaReiser marked this pull request as ready for review October 17, 2023 06:29
@MichaReiser MichaReiser mentioned this pull request Oct 17, 2023
@MichaReiser
Copy link
Member Author

@charliermarsh what's your preferred way to incorporate these extensions to the documentation? Should I open a PR to your documentation PR or do you want to write the documentation yourself?

crates/ruff_cli/src/commands/check.rs Outdated Show resolved Hide resolved
Comment on lines +197 to +208
let (mut all_diagnostics, checked_files) = diagnostics_per_file
.fold(
|| (Diagnostics::default(), 0u64),
|(all_diagnostics, checked_files), file_diagnostics| {
(all_diagnostics + file_diagnostics, checked_files + 1)
},
)
.reduce(
|| (Diagnostics::default(), 0u64),
|a, b| (a.0 + b.0, a.1 + b.1),
);
Copy link
Member

Choose a reason for hiding this comment

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

This needs a comment what it does

crates/ruff_cli/tests/format.rs Outdated Show resolved Hide resolved
crates/ruff_workspace/src/options.rs Show resolved Hide resolved
crates/ruff_workspace/src/resolver.rs Outdated Show resolved Hide resolved
Allow excluding files from the formatter and linter.
@MichaReiser MichaReiser enabled auto-merge (squash) October 18, 2023 01:05
@MichaReiser MichaReiser merged commit fe485d7 into main Oct 18, 2023
15 of 30 checks passed
@MichaReiser MichaReiser deleted the format-lint-exclude-option branch October 18, 2023 01:15
@MichaReiser MichaReiser restored the format-lint-exclude-option branch October 18, 2023 01:16
@MichaReiser MichaReiser deleted the format-lint-exclude-option branch January 16, 2024 10:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
configuration Related to settings and configuration formatter Related to the formatter
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add exclude option to format and lint sections
4 participants