- 
                Notifications
    
You must be signed in to change notification settings  - Fork 1.6k
 
          Add [format|lint].exclude options
          #8000
        
          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
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.
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
| /// 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.
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)
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 
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.
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
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.
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
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.