Conversation
|
Current dependencies on/for this PR:
This comment was auto-generated by Graphite. |
| return Ok(Diagnostics::default()); | ||
| } | ||
|
|
||
| let lint_settings = &pyproject_config.settings.linter; |
There was a problem hiding this comment.
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
| /// The lint sections specified at the top level. | ||
| #[serde(flatten)] | ||
| pub lint_top_level: LintOptions, | ||
| pub lint_top_level: LintCommonOptions, |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
Can this be removed when we remove the top-level lint options support?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
lintsection over theformattersection (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
| pyproject_config: &PyprojectConfig, | ||
| transformer: &dyn ConfigurationTransformer, | ||
| ) -> Result<(Vec<Result<DirEntry, ignore::Error>>, Resolver)> { | ||
| ) -> Result<(Vec<Result<ResolvedFile, ignore::Error>>, Resolver)> { |
There was a problem hiding this comment.
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
dafec38 to
e7ec415
Compare
PR Check ResultsEcosystem✅ ecosystem check detected no changes. |
e7ec415 to
b3df3f8
Compare
b3df3f8 to
fe03d95
Compare
|
@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? |
| 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), | ||
| ); |
There was a problem hiding this comment.
This needs a comment what it does
Allow excluding files from the formatter and linter.
fe03d95 to
499200f
Compare

Summary
This PR introduces the new options
lint.excludeandformat.excludethat allow excluding files from linting or formatting only.Being able to exclude entire files is useful when:
Closes #7645
Considerations
The
checkcommand (and now the format command) exposes theexcludeoption. This is the globalexcludeand not thelint.excludeorformat.exclude. Only exposing the globalexcludeoption should be sufficient and make little difference except if the user wants to clear out thelint|format.excludeoption via the CLI.Only exposing the global
excludeoption makes sense for me, considering that we plan to unify linting and formatting under thecheckcommand in the future. Whether this is the right design forformatis less clear to me but I think it's in line withcheckand we can still decide to add aformat-excludeCLI option if it proves necessary.Test Plan
I added new integration tests that test the tool specific exclusion.
I played around with the
airflowrepository 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.