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

Extend options for typos hook #392

Merged
merged 3 commits into from
Feb 15, 2024

Conversation

totoroot
Copy link
Collaborator

@totoroot totoroot commented Jan 16, 2024

Adding options for existing flags

I've added a few more options to the typos hook to pass existing command line flags.
These options are binary, no-check-filenames, no-check-files, no-unicode, quiet, verbose.
I also refactored the descriptions of the options and cmdArgs of the hook a bit.

Adding an option to ignore files

I asked in the typos repo how to best ignore words, that should not be marked as typos. The way typos works, you have to add these words/spellings to the word list by extending it.
Due to some design decisions, typos maintainers will not introduce CLI flags to ignore words and will to stick to the use of a single config file (_typos.toml) for that.

To make typos even more usable as a pre-commit hook, I've added an option to list words to be ignored in the configuration of the hook. This makes it possible to set the following:

settings.typos.ignored-words = [
  "mqtt"
  "mosquitto"
  ];

If the option is set, the list of words then gets appended to a passed config file in a way typos understands.
The resulting config file passed to the hook then contains the required section:

[default.extend-words]
mqtt = "mqtt"
mosquitto = "mosquitto"

If both settings.typos.config and settings.typos.configPath are set, the latter gets ignored. This is now documented in the option's description, so that users know not to configure both.

Unsetting pass_filenames

In commit f441d08, I have removed the recent change from @phip1611. I stated in the PR discussion why I think changing this was not a good idea.
@domenkozar Perhaps we can wait for @phip1611's response and discuss this in #387 before this PR gets merged.

@phip1611
Copy link
Contributor

phip1611 commented Jan 16, 2024

Please consider adding this patch.

Discussion is in the issue #387.

@totoroot totoroot marked this pull request as draft January 16, 2024 12:57
@phip1611
Copy link
Contributor

@totoroot any update? I'm only available for one more week before I'm on vacation; I hope we can merge this soon :)

@domenkozar
Copy link
Member

Ping :)

@totoroot
Copy link
Collaborator Author

Sorry, for the late response. I missed the notifications on this one.

@phip1611 I tried incorporating your patch, but did not see how this fixes the issues that only came up after #387 got merged.
As @Stunkymonkey has also reported this change breaking their config in #387, I'd still suggest reverting the change by merging commit f441d08 as par of this PR.
Then you can open up a follow-up PR with your proposed patch and we can thoroughly test it and see whether the hook then works for you, @Stunkymonkey and me.

@domenkozar Is that alright?

@totoroot totoroot marked this pull request as ready for review February 15, 2024 09:10
@domenkozar domenkozar merged commit 5df5a70 into cachix:master Feb 15, 2024
3 of 4 checks passed
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.

3 participants