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

Excluding files unexpected behavior. #347

Closed
AIGeneratedUsername opened this issue Sep 15, 2021 · 22 comments · May be fixed by fuzzylabs/MindGPT#73
Closed

Excluding files unexpected behavior. #347

AIGeneratedUsername opened this issue Sep 15, 2021 · 22 comments · May be fixed by fuzzylabs/MindGPT#73
Labels
bug Not as expected

Comments

@AIGeneratedUsername
Copy link

Current settings:

_typos.toml

[files]
extend-exclude = ["*.json", "**/*.json"]

.pre-commit-config.yaml

-   repo: https://github.com/crate-ci/typos
    rev: typos-v0.7.4
    hooks:
      - id: typos

Expected behavior:
JSON files must be excluded

Expected behavior:
All files are checked.

Steps to reproduce

pre-commit run typos --all-files

@epage epage added the bug Not as expected label Sep 15, 2021
@epage
Copy link
Collaborator

epage commented Sep 15, 2021

typos assumes the user knows best when passing in file paths, overriding the exclusions. Unfortunately, this doesn't layer well with tools like pre-commit.

We need to provide a way to control this behavior.

@amitlevy21
Copy link

I have the same issue, but only when running typos-src with pre-commit, running typos from the terminal works fine.
this is my config (_typos.toml at project root directory):

[files]
extend-exclude = ["src/assets/locales"]

locale files are ignored when running from the terminal, but not from pre-commit run --all-files -v typos-src

  - repo: https://github.com/crate-ci/typos
    rev: v1.3.3
    hooks:
      - id: typos-src
        # args:
        #   - --exclude
        #   - src/assets/locales #uncommenting args works, but its not reading my config file

@kwanhur
Copy link

kwanhur commented Mar 6, 2022

typos assumes the user knows best when passing in file paths, overriding the exclusions. Unfortunately, this doesn't layer well with tools like pre-commit.

We need to provide a way to control this behavior.

What about the hook attribute exclude ? It can exclude files with specified pattern.

@TomaszAIR

This comment was marked as resolved.

@epage

This comment was marked as resolved.

@epage

This comment was marked as resolved.

@TomaszAIR

This comment was marked as resolved.

@TomaszAIR

This comment was marked as resolved.

@sshishov
Copy link

Hello, when we can expect the fix for the issue? Just a question, not pushing for any actions 😉

@epage
Copy link
Collaborator

epage commented May 23, 2022

Hello, when we can expect the fix for the issue?

The core of this is we need a natural mode where we trust the user and a scripted mode where we don't trust the user. The blocker is defining how to express this in the UI. Bonus points for including any precedence from other tools

Also want to note that #461 has the workaround of duplicating the exclusions

@sshishov
Copy link

Thanks @epage , currently as a workaround we started using pre-commit exclude, but added #TODO: with appropriate comment. And also keep the configuration to exclude in the config file

@sshishov
Copy link

Additional workaround is:

  - repo: https://github.com/crate-ci/typos
    rev: v1.8.1
    hooks:
    - id: typos
      pass_filenames: false

Got it from here as the same issue exists in interrogate package

@sshishov
Copy link

The same issue as in isort fixed by PR 939 and black fixed by PR 1032

Same problem was happening with flake8 and happening now with [interrogate](https://github.com/crate-ci/typos/issues/347)

@epage
Copy link
Collaborator

epage commented May 29, 2022

@sshishov thanks for those examples. To summarize:

  • isort added --filter-files
  • black added --force-exclude

I wish there was a flag that was common between tools, even if the sample size is just two, as being consistent can be more important than being ideal on its own.

Since these aren't consistent, my next thought is on which more clearly communicates intent. I feel like isort's only makes sense when reading the docs and thinking about what it is trying to say. black's makes a little more sense. "force" make it clear this is a binary flag and that it is overriding some default behavior. I lean towards that somewhat even though something still feels off to me. Really, we are implicitly forcing the the args to be respected independent of the exclusions. I just can't think of a succinct way of saying that, so maybe just mirroring black would be best.

If people know of other examples, I'm still open open on this. At least for me adding the flag, it might be a week or so as I'm trying to wrap up some major work on the clap crate.

@arminrosu
Copy link

The core of this is we need a natural mode where we trust the user and a scripted mode where we don't trust the user. The blocker is defining how to express this in the UI. Bonus points for including any precedence from other tools

Here's another example - prettier ignores all files that are in it's ignore list, even if you explicitly tell it to format that particular file.

AFAIK, this behaviour exists because you're expected to run prettier on a folder or multiple files and rarely run it on a single file.

That is a pattern I'd like to see in typos as well, but it's up to you. I think it's easier to have 2 configs - one with a blacklist and one without one, than adding multiple cli flags to ignore/force unignore configuration settings.


My original issue was with running typos as a github action on changed files, which then spellchecks blacklisted files as well.

It would have been possible to filter out the blacklist from the changelist, if the exclude config was in a format bash can parse natively. Of course, I can install toml parsers, filter out the blacklist from the changeset and pass that along to typos...


Btw, thanks for closing the issue I opened and redirecting me here. I didn't find this issue initially.

scop added a commit to scop/pytekukko that referenced this issue Aug 31, 2022
This would be better placed in `.typos.toml` rather than pre-commit
config, but at time of writing, `typos` doesn't honor exclusions for
explicitly passed files: crate-ci/typos#347
scop added a commit to scop/pylttoaine that referenced this issue Aug 31, 2022
This would be better placed in `.typos.toml` rather than pre-commit
config, but at time of writing, `typos` doesn't honor exclusions for
explicitly passed files: crate-ci/typos#347
scop added a commit to scop/pylttoaine that referenced this issue Sep 27, 2022
This would be better placed in `.typos.toml` rather than pre-commit
config, but at time of writing, `typos` doesn't honor exclusions for
explicitly passed files: crate-ci/typos#347
@adamchainz
Copy link

What about the hook attribute exclude ? It can exclude files with specified pattern.

This is what I'm using. For example:

repos:
- repo: https://github.com/crate-ci/typos
  rev: v1.13.10
  hooks:
    - id: typos
      exclude: |
        (?x)^(
          .*\.csv
          |.*\.xml
        )$

You can also explicitly limit to certain file types by overriding the text type selected in the repo's hook definition:

repos:
- repo: https://github.com/crate-ci/typos
  rev: v1.13.10
  hooks:
    - id: typos
      types_or:
      - javascript
      - python

IMO one of the powers of pre-commit is that it already passes the correct filenames, based on hook selection and exclusion, so tools don't have to reimplement undifferentiated inclusion/exclusion features.

@epage
Copy link
Collaborator

epage commented Feb 7, 2023

IMO one of the powers of pre-commit is that it already passes the correct filenames, based on hook selection and exclusion, so tools don't have to reimplement undifferentiated inclusion/exclusion features.

For tools written exclusively for pre-commit, that works well. However, this tool is written more generally and needs to have its own logic.

epage added a commit to epage/typos that referenced this issue Aug 21, 2023
This reverts commit fc7f517.

This has two problems
- This doesn't correctly handle spaces, likely needing crate-ci#708
- This overrides excludes, see crate-ci#347

This also has a weird cost/benefit balance because this requires enough
repo history to do the analysis which takes time to pull down.

Rather than waiting until the relevant changes are in to make this work,
I'm pulling this out for now.

Fixes crate-ci#806
@Delgan
Copy link
Contributor

Delgan commented Sep 24, 2023

Hi @epage.

Would you accept a PR adding a new --force-exclude similar to the one used by black?

--force-exclude
Like --exclude, but files and directories matching this regex will be excluded even when they are passed explicitly as arguments. This is useful when invoking Black programmatically on changed files, such as in a pre-commit hook or editor plugin.

I really wish I could use typos with pre-commit without the pass_filenames: false workaround.


On a second thought, the --force-exclude of black seems overkill since it expects a regex. A boolean flag looks more appropriate.

I think by default, we want typos to ignore excluded files even if they're explicitly passed to the command line. This is because using typos *.py is a very common usage, so it makes sense that exclude takes precedence.

Since typos already has some command line options such as --no-ignore, maybe we could just add --no-exclude to ignore the configured extend-exclude list?

@epage
Copy link
Collaborator

epage commented Sep 25, 2023

Yes, when looking at black before, I thought it was a bool and not yet another exclude type which seems like it'd be awkward to deal with.

I think by default, we want typos to ignore excluded files even if they're explicitly passed to the command line. This is because using typos *.py is a very common usage, so it makes sense that exclude takes precedence.

Is typos *.py very common? For myself, I run it on directories or on individual files and neither case runs into this problem.

If we went this route, it'd be a breaking change and we'd need to bump to 2.0. Not the end of the world but something to keep in mind.

I also feel like we'd need a way to help the user discover that we ignored what they said and they need to pass in an extra flag. Not thrilled with doing that.

Most of this problem seems to be centered on pre-commit-like workflows where we shouldn't be trusting the external source of information.

@Delgan
Copy link
Contributor

Delgan commented Sep 26, 2023

I've done more research and compared other tools, and I admit that what I've suggested isn't that common. It depends on people's preferences, but there doesn't seem to be any consensus.
Personally, I'm more likely to use typos *.py than to call typos on a file I've explicitly configured to be excluded.

Just for example, there is ESLint which displays a warning when excluded files are passed to the command line and requires an explicit --no-ignore: Ignore Files - ESLint - Pluggable JavaScript Linter.

However if we take into account the issue regarding backwards compatibility, I agree it's definitely preferable not to change the existing behavior. Apologies for not thinking of that.

As a further example, there is also Ruff which implemented --force-exclude as a boolean flag and directly integrated it to their pre-commit hook: ruff-pre-commit/.pre-commit-hooks.yaml#L4.
This matches with solutions discussed in this thread and we could copy it for a touch of consistency.

I can understand your hesitations to add such a flag, but its primary purpose is to enable typos to be used programmatically. Not only for pre-commit, but also to any Bash script or VS Code extension that wants to use typos without the burden of maintaining its own exclude list.
From my perspective, it's more of a valuable feature than a mere workaround.

I am always willing to open a PR if you wish.

@epage
Copy link
Collaborator

epage commented Sep 26, 2023

My concern with --force-exclude was more centered on black having it be a separate list of excludes. Having it be a bool would make sense and I'd be willing to be consistent with ruff in this.

epage added a commit that referenced this issue Oct 9, 2023
@epage
Copy link
Collaborator

epage commented Oct 9, 2023

Closed via #837

@epage epage closed this as completed Oct 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Not as expected
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants