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.

Sign up for GitHub

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
Open

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

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f826a04
typos: fix unexpected behaviour
phip1611 Mar 19, 2024
46b368e
typos: add force-exclude setting (default true)
phip1611 Apr 19, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
69 changes: 62 additions & 7 deletions modules/hooks.nix
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1436,7 +1436,7 @@ in
configuration =
mkOption {
type = types.str;
description = lib.mdDoc "Multiline-string configuration passed as config file. If set, config set in `typos.settings.configPath` gets ignored.";
description = lib.mdDoc "Multiline-string configuration passed as config file. It is recommended to use `configPath` instead for a more natural experience of typos.";
default = "";
example = ''
[files]
Expand All @@ -1450,11 +1450,14 @@ in
'';
};

# It is recommended to use a Nix path here as this way, the excludes
# from the config file can be taken into account by pre-commit when
# running `$ pre-commit run --all-files`.
configPath =
mkOption {
type = types.str;
description = lib.mdDoc "Path to a custom config file.";
default = "";
type = types.nullOr (types.either types.str types.path);
description = lib.mdDoc "[Path](https://nixos.org/manual/nix/stable/language/values#type-path) to a typos config file (recommended) or a string (deprecated)";
default = null;
example = ".typos.toml";
};

Expand All @@ -1473,6 +1476,15 @@ in
example = "*.nix";
};

force-exclude =
mkOption {
type = types.bool;
description = lib.mdDoc "Respect excluded files from config file even for paths passed explicitly";
# This must be true as this is the natural behaviour. Similar to when executing `typos`
# from the cmdline.
default = true;
};

format =
mkOption {
type = types.enum [ "silent" "brief" "long" "json" ];
Expand Down Expand Up @@ -3314,6 +3326,47 @@ lib.escapeShellArgs (lib.concatMap (ext: [ "--ghc-opt" "-X${ext}" ]) hooks.ormol
entry = "${hooks.treefmt.package}/bin/treefmt --fail-on-change";
};
typos =
let
# Path to config file. May be in Nix store but can also be a relative
# path to a system-dependent file.
#
# After various upstream discussions [0]), the best solution is to
# provide the config file as Nix path but keep the string passing
# as fall-back.
#
# [0]: https://github.com/cachix/pre-commit-hooks.nix/pull/387#issuecomment-1893600631
pathToConfigFile =
if hooks.typos.settings.configPath != null
# Important: If passed as typeOf == "path", this is in Nix store
# which we in fact encourage.
then hooks.typos.settings.configPath
else
builtins.toFile "config.toml"
# Concatenate config in config file with section for ignoring words
# generated from list of words to ignore
(hooks.typos.settings.configuration +
lib.strings.optionalString (hooks.typos.settings.ignored-words != [ ]) "\n\[default.extend-words\]" +
lib.strings.concatMapStrings (x: "\n${x} = \"${x}\"") hooks.typos.settings.ignored-words
)
;
# If the config file path is passed as string and not as path, we
# can't read it from Nix.
excludesFromConfig =
if builtins.typeOf hooks.typos.settings.configPath == "path" # passed directly or as Path
then
(
let
toml = builtins.fromTOML (builtins.readFile pathToConfigFile);
in
/*
The "files.extend-exclude" key comes from
https://github.com/crate-ci/typos/blob/master/docs/reference.md
*/
(toml.files or { }).extend-exclude or [ ]
)
else
[ ];
in
{
name = "typos";
description = "Source code spell checker";
Expand All @@ -3328,10 +3381,11 @@ lib.escapeShellArgs (lib.concatMap (ext: [ "--ghc-opt" "-X${ext}" ]) hooks.ormol
(with hooks.typos.settings; [
[ binary "--binary" ]
[ (color != "auto") "--color ${color}" ]
[ (configuration != "") "--config ${configFile}" ]
[ (configPath != "" && configuration == "") "--config ${configPath}" ]
# Config file always exists (we generate one if not).
[ true "--config ${pathToConfigFile}" ]
[ diff "--diff" ]
[ (exclude != "") "--exclude ${exclude} --force-exclude" ]
[ (exclude != "") "--exclude ${exclude}" ]
[ (force-exclude) "--force-exclude" ]
[ (format != "long") "--format ${format}" ]
[ hidden "--hidden" ]
[ (locale != "en") "--locale ${locale}" ]
Expand All @@ -3345,6 +3399,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 •
edited
Loading

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 •
edited
Loading

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 •
edited
Loading

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.

};
typstfmt = {
name = "typstfmt";
Expand Down