Add "--force-exclude" option #837
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
crates/typos-cli/src/config.rs
Outdated
| @@ -98,6 +98,8 @@ impl Config { | |||
| #[serde(rename_all = "kebab-case")] | |||
| pub struct Walk { | |||
| pub extend_exclude: Vec<String>, | |||
| // Exclude files even if passed explicitly. | |||
| pub force_exclude: Option<bool>, | |||
There was a problem hiding this comment.
To me, this feature seems more situational rather than being a fixed state, so I think it should be cli only and not in config
There was a problem hiding this comment.
I agree. I moved force_exclude from WalkArgs to Args (since parsed WalkArgs are only publicly accessible through Config). This implies means that --force-exclude option will not appear immediately after --exclude in the --help.
There was a problem hiding this comment.
I think that'll be ok. I'm working on #846 which will make it easier to review help output changes and, in looking at it, I think this works well for the new top section
| @@ -216,6 +216,12 @@ fn run_checks(args: &args::Args) -> proc_exit::ExitResult { | |||
| let overrides = overrides | |||
| .build() | |||
| .with_code(proc_exit::sysexits::CONFIG_ERR)?; | |||
| if walk_policy.force_exclude() && path.is_file() { | |||
There was a problem hiding this comment.
Good question... I think I over-focused on my specific use case, sorry.
| @@ -216,6 +216,12 @@ fn run_checks(args: &args::Args) -> proc_exit::ExitResult { | |||
| let overrides = overrides | |||
| .build() | |||
| .with_code(proc_exit::sysexits::CONFIG_ERR)?; | |||
| if walk_policy.force_exclude() && path.is_file() { | |||
| match overrides.matched(path, false) { | |||
There was a problem hiding this comment.
Something I need to double check is that some of these APIs expect you to call them as you recurse into a directory and return confusing results if you only call them on the final path.
There was a problem hiding this comment.
Hum... I think you're correct. If I have foo/bar/baz.file directory structure and extend-exclude = ["bar/*.file"] configuration, using typos foo will ignore the file properly but typos foo/bar/baz.file --force-exclude won't (using extend-exclude = ["foo/bar/*.file"] or extend-exclude = ["baz.file"] works, though). 😕
Not sure how to fix it. Maybe we should propose a feature to ignore crate so that it optionally checks root entry as well: walk.rs#L936-L938?
I don't know how likely is it to be accepted, knowing that Walk is supposed to be used with directory as argument (from the docs at least), and we're using it with file path as well.
There was a problem hiding this comment.
You likely need to do path.ancestors().rev(), calling matched` on each
There was a problem hiding this comment.
Apologies, I think I just did something wrong during my test. The file is not excluded in any case, which is the expected behavior.
|
Sorry about the spelling failure in CI; I had a bad merge which introduced this workflow when it shouldn't be here. Fixed in master |
No problem. I rebased the PR. 👍 |
|
Thanks! |
Hi.
As discussed in #347, I'm proposing here a simple
--force-excludeoption if that sounds good to you.Regarding the implementation, I had to use
Override.matched()because theWalkclass unconditionally includes the initial path it receives. This means that any explicitly passed file we might want to exclude would not have a chance to be filtered.I'm not very familiar with Rust so let me know if there are things you want me to improve.