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

typos: allow configPath to be of type string or path + take excludes from .typos.toml into account when pre-commit run typos --all-files runs #405

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

phip1611
Copy link
Contributor

@phip1611 phip1611 commented Mar 12, 2024

Unfortunately, the typos story still isn't solved properly at this point. My concerns decsribed here still exist.

The Problem

Hint: The problem is also shortly discussed in the commit messages.

Yes, using pass_filenames = false should only be done if no other variant is possible (pass_filenames = true is fine). My first "fix" merged a few months ago was wrong. And a commit hook should only run on changed files, I agree.

But, people also use pre-commit run --all-files to run all checks on the whole repo. Some people don't even use Git commit hooks at all and just use pre-commit to run all style checks at once. And that's a perfeclty valid usecase.

The problem is that if you run $ pre-commit run typos --all-files, typos behave differently compared to $ typos ..

The Fix

I modified typos.configPath in a way to be either of type string or path. The change is additive and not breaking.

To use the new option, you have to do something like this:

    settings = {
      typos = {
        # configuration = ""  # POSSIBLE
        # configPath = ".typos.toml"; # OLD
        configPath = .typos.toml; # NEW, RECOMMENDED
      };
    };

You can verify that my solution works by checking the typos invocation:

$ strace -s 64 -Tfe trace=execve pre-commit run --all-files typos

Unintended files are not longer passed to typos. 🎉


Ping @totoroot @domenkozar

@phip1611 phip1611 changed the title typos: fix behaviour typos: use excludes from .typos.toml when pre-commit run typos runs Mar 12, 2024
@phip1611 phip1611 changed the title typos: use excludes from .typos.toml when pre-commit run typos runs typos: use excludes from .typos.toml when pre-commit run typos --all-files runs Mar 12, 2024
@phip1611
Copy link
Contributor Author

FYI ping @blitz

@phip1611
Copy link
Contributor Author

phip1611 commented Mar 12, 2024

Personally, I'd also like to get rid of configPath of type str, but then we break the API again.

configPath is now either string or path.

@totoroot
Copy link
Collaborator

Sounds good. I'll test it in the coming days.

@domenkozar
Copy link
Member

Sorry for the mess, we revamed module structure in #397, could you port these changes over?

@phip1611 phip1611 closed this Mar 19, 2024
@phip1611 phip1611 reopened this Mar 19, 2024
@phip1611 phip1611 marked this pull request as draft March 19, 2024 11:46
@phip1611 phip1611 marked this pull request as ready for review March 19, 2024 11:50
@phip1611
Copy link
Contributor Author

@domenkozar done

@domenkozar
Copy link
Member

ping @totoroot for testing

@phip1611
Copy link
Contributor Author

phip1611 commented Apr 2, 2024

I rebased again and improved my solution. My solution is additive and doesn't break anything; I tested all the existing use cases. Can we merge it, @domenkozar ?

@phip1611 phip1611 changed the title typos: use excludes from .typos.toml when pre-commit run typos --all-files runs typos: allow configPath to be of type string or path + take excludes from .typos.toml into account when pre-commit run typos --all-files runs Apr 2, 2024
modules/hooks.nix Outdated Show resolved Hide resolved
Copy link

@astro astro left a comment

Choose a reason for hiding this comment

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

LGTM apart from a few nits

modules/hooks.nix Outdated Show resolved Hide resolved
modules/hooks.nix Outdated Show resolved Hide resolved
modules/hooks.nix Outdated Show resolved Hide resolved
@phip1611
Copy link
Contributor Author

phip1611 commented Apr 19, 2024

Ping @m1-s @domenkozar @totoroot - I hope we can finally get this merged as we need it in multiple of our projects at Cyberus Technology :)

@domenkozar
Copy link
Member

If there's no comments in next few days I'll merge it.

Copy link
Contributor

@roberth roberth left a comment

Choose a reason for hiding this comment

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

Was mostly interested in the usage of path values, but also found a bug.

@@ -3331,6 +3375,7 @@ lib.escapeShellArgs (lib.concatMap (ext: [ "--ghc-opt" "-X${ext}" ]) hooks.ormol
in
"${hooks.typos.package}/bin/typos ${cmdArgs}";
types = [ "text" ];
excludes = excludesFromConfig;
Copy link
Contributor

Choose a reason for hiding this comment

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

excludes is expected to be in regex form, whereas excludesFromConfig has globs.

Copy link
Contributor Author

@phip1611 phip1611 Apr 19, 2024

Choose a reason for hiding this comment

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

From my personal testing I can verify that it works when the typos.toml file doesn't use globs.

What can we do do mitigate this issue? This issue really needs to be solved for some of our projects. Any idea?

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe look for *s and ?s and if they occur, throw an explanation that it's not implemented?
Not fantastic, but leaps better than making users debug or keeping your colleagues/clients waiting.

Ideally, the globs could be translated. https://github.com/hercules-ci/gitignore.nix has inherited an implementation of that from nix-gitignore, but I don't know if it's the same dialect if any. It translates globs into a regex, using a regex... :D ("so now I have three problems"?)

Copy link
Contributor Author

@phip1611 phip1611 Apr 19, 2024

Choose a reason for hiding this comment

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

I updated the branch and updated the commit messages.

I think removing everything that looks like a glob from exclude-list is fine and good-enough. It is clearly a benefit compared to the current situation.

Will do it on Monday. Happy weekend :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Are you suggesting to silently drop excludes? I would hate that as a user and flip a table ;)

Likewise!

Copy link
Contributor Author

@phip1611 phip1611 Apr 22, 2024

Choose a reason for hiding this comment

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

Agree. Yikes, the whole thing is complicated. I just want typos always behave the same, with and without pre-commit run. The current behavior means trouble and manual workarounds for at least one of our internal projects.

See #405 (comment) for a summary of the current discussion.

modules/hooks.nix Outdated Show resolved Hide resolved
phip1611 added a commit to phip1611/pre-commit-hooks.nix that referenced this pull request Apr 19, 2024
This came up after a long discussion in [0]. Using this (as default option)
is probably the simplest solution and the most natural behaviour.

[0]: cachix#405
phip1611 added a commit to phip1611/pre-commit-hooks.nix that referenced this pull request Apr 19, 2024
Although that is not the solution for the performance problems
(in form of huge amounts of resources are consumed) of `typos`
discussed in [0] and referenced discussions, this is technically
the right thing to do and what users expect.

[0]: cachix#405
phip1611 added a commit to phip1611/pre-commit-hooks.nix that referenced this pull request Apr 19, 2024
Although that is not the solution for the performance problems
(in form of huge amounts of resources are consumed) of `typos`
discussed in [0] and referenced discussions, this is technically
the right thing to do and what users expect.

[0]: cachix#405
Enable typos.configPath to be of type path. This way, we can
use excludes from .typos.toml when running
`pre-commit run typos --all-files`. Otherwise, the execution
differs from a normal invocation of typos, which is unexpected
and leads to wrong results.

To not break anything, and to be compliant with the existing
API, I modified configPath to be either a Nix path or a string.

The main motivation for this change is a repository on my machine
where pre-commit ignores directory foobar (which is excluded
by .typos.toml) but passes all 65,000 files of foobar as argument
to typos. In these situation, typos goes nuts and often the
system freezes.

So with this change, I prevent that possibly tens of thousands
of files that should not be checked in any case are passed to
typos, which results in a smooth experience.

[0]: cachix#387 (comment)
phip1611 added a commit to phip1611/pre-commit-hooks.nix that referenced this pull request Apr 19, 2024
Although that is not the solution for the performance problems
(in form of huge amounts of resources are consumed) of `typos`
discussed in [0] and referenced discussions, this is technically
the right thing to do and what users expect.

[0]: cachix#405
Although that is not the solution for the performance problems
(in form of huge amounts of resources are consumed) of `typos`
discussed in [0], referenced discussions, and the previous commit,
this is technically the right thing to do and what users expect.

[0]: cachix#405
@phip1611
Copy link
Contributor Author

phip1611 commented Apr 22, 2024

It's a little unfortunate that the whole topic got so complex.

Intermediate summary

I'm working towards that $ pre-commit run [--all-files] behaves similar to $ typos with the exact same set of excludes as this is the correct thing to do.

However, in this whole long process, we discovered/faced/targeted the following problems:

  • typos usually is not supposed to be invoked with arguments, but the natural way is typos . (config file is implicitly used from the project root). You can use it with arguments but that is not what I've seen - it is not broadly used
  • pre-commit passes files directly as arguments to tools such as typos
  • Some folks didn't want to set pass_filenames = false for typos
  • When typos is invoked with thousands of files (we have that in one of our projects, and we excluded that directory as it is third party code) as argument it literally explodes and CPU and mem consumption are insane; system freezes happen (even with --force-exclude)
  • For exlusions, typos uses globs, pre-commit uses regex
  • If .typos.toml uses "globs without *", my approach presented in this PR works and it's fine.
  • The transformation from globs to regex is not trivial.

Observation/Problems/Next steps

  1. IMHO this is critical and must be addressed to not annoy and confuse people and to not make them drop typos support via pre-commit or use workarounds
  2. I do not see a pragmatic way forward here. Such a rather simple thing is already a time sink. I want to satisfy everyones needs. But I can't spend multiple days on this, unfortunately.

This needs to be addressed. The alternative is that we at my Company manually specify excludes locally and that I create a blogpost or so for typos in pre-commit-hooks.nix for other people having the same problem.

ping @roberth - Thoughts?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants