feat: support specifying stages #52

blaggacao · 2020-07-20T21:38:35Z

closes #49

Since I'm a newcomer, I have no way to judge (or even validate) if that's the right way at all: it's a shot into the darknix.
If it's a hit, I appreciate that I could seamlessly contribute. If it's a miss, please guild me around to get this merge-ready.

roberth · 2020-07-20T21:43:27Z

darknix

nice :D

I think it's missing the option declaration. I haven't read the pre-commit docs so maybe it's not accurate, but perhaps excludes can serve as an example https://github.com/cachix/pre-commit-hooks.nix/pull/52/files#diff-0920dde2a9dc0ea9c564d0c0131c8feeR251

You can test your change by using it in a project. We don't have a test suite yet.

roberth · 2020-07-20T21:44:43Z

You can replace the builtins.fetchTarbal "..." part in the README by an unquoted local path like ../pre-commit.nix for testing

blaggacao · 2020-07-20T21:45:53Z

Thanks! I broke things. 😢

nix-shell --show-trace
warning: unknown setting 'experimental-features'
unpacking 'https://github.com/blaggacao/pre-commit-hooks.nix/tarball/master'...
error: while evaluating the attribute 'lorriHook' of the derivation 'nix-shell' at /nix/store/isyka0a95cxhj1381bg3bbqdv78qrln9-nixpkgs-20.09pre235189.d7e20ee25ed/nixpkgs/pkgs/build-support/mkshell/default.nix:28:3:
while evaluating the attribute 'pre-commit-check.shellHook' at /home/blaggacao/ghq/git.p1.rocks/gitops/aedir-ldap.k8s/default.nix:6:3:
while evaluating anonymous function at /nix/store/b4jq885hsxy31h23f3b6c5dnizchgn5q-source/nix/run.nix:3:1, called from /home/blaggacao/ghq/git.p1.rocks/gitops/aedir-ldap.k8s/default.nix:6:22:
while evaluating the attribute 'config.pre-commit.run' at undefined position:
while evaluating anonymous function at /nix/store/ma3b77vjls3qrznwh66ix5ivvybx6003-source/lib/modules.nix:84:45, called from undefined position:
while evaluating 'yieldConfig' at /nix/store/ma3b77vjls3qrznwh66ix5ivvybx6003-source/lib/modules.nix:83:29, called from /nix/store/ma3b77vjls3qrznwh66ix5ivvybx6003-source/lib/modules.nix:86:16:
while evaluating anonymous function at /nix/store/ma3b77vjls3qrznwh66ix5ivvybx6003-source/lib/modules.nix:89:24, called from /nix/store/ma3b77vjls3qrznwh66ix5ivvybx6003-source/lib/modules.nix:89:11:
while evaluating anonymous function at /nix/store/ma3b77vjls3qrznwh66ix5ivvybx6003-source/lib/modules.nix:90:26, called from /nix/store/ma3b77vjls3qrznwh66ix5ivvybx6003-source/lib/modules.nix:90:13:
The option `pre-commit.default_stages' defined in `<unknown-file>' does not exist.

Yeay! I'm trying very hard to read between the line, yet this log doesn't talk to my brain.

roberth · 2020-07-20T21:47:10Z

Oh my link above is broken. You need to declare an option for default_stages with mkOption. It's using the NixOS module system.

modules/pre-commit.nix

blaggacao · 2020-07-20T22:16:07Z

Example result

───────┬───────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
       │ File: .pre-commit-config.yaml
───────┼───────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
   1   │ # DO NOT MODIFY
   2   │ # This file was generated by nix-pre-commit-hooks
   3   │ {
   4   │   "default_stages": [
   5   │     "manual",
   6   │     "pull"
   7   │   ],
   8   │   "exclude": "(^k8s/kustomize/|^images/ansible-ae-dir-server/|^images/config-initializer/ae-dir-configuration.sample.yml$|^images/config-initializer/ae-dir-customization.yml$|^images/config-initializer/ae-dir-container.yml$)",
   9   │   "repos": [
  10   │     {
  11   │       "hooks": [
  12   │         {
  13   │           "entry": "/nix/store/2pwj46x0qq9asnxkag1qcbvkz2zf2a9w-kubeval-0.15.0/bin/kubeval",
  14   │           "exclude": "^$",
  15   │           "files": "^k8s/.*?\\.(yaml|yml)$",
  16   │           "id": "kubeval",
  17   │           "language": "system",
  18   │           "name": "kubeval",
  19   │           "pass_filenames": true,
  20   │           "types": []
  21   │         },
  22   │         {
  23   │           "entry": "/nix/store/fisrj8c6cizy9df8hbg9cz1y64yvb1bv-node_prettier-2.0.5/bin/prettier --write --list-different",
  24   │           "exclude": "^$",
  25   │           "files": "\\.(css|less|scss|graphql|gql|html|js|jsx|json|md|markdown|mdown|mkdn|mdx|ts|tsx|vue|yaml|yml)$",
  26   │           "id": "prettier",
  27   │           "language": "system",
  28   │           "name": "prettier",
  29   │           "pass_filenames": true,
  30   │           "types": []
  31   │         },
  32   │         {
  33   │           "entry": "/nix/store/000llwy4g16jhmbi3yzngyzp1k4qykp0-ShellCheck-0.7.0/bin/shellcheck",
  34   │           "exclude": "^$",
  35   │           "files": "",
  36   │           "id": "shellcheck",
  37   │           "language": "system",
  38   │           "name": "shellcheck",
  39   │           "pass_filenames": true,
  40   │           "types": [
  41   │             "bash"
  42   │           ]
  43   │         },
  44   │         {
  45   │           "entry": "/nix/store/zihp6pxl6s8cb45zfbn2hcqkhis2zsnf-shfmt-3.1.2/bin/shfmt -w -l",
  46   │           "exclude": "^$",
  47   │           "files": "",
  48   │           "id": "shfmt",
  49   │           "language": "system",
  50   │           "name": "shfmt",
  51   │           "pass_filenames": true,
  52   │           "types": [
  53   │             "shell"
  54   │           ]
  55   │         }
  56   │       ],
  57   │       "repo": "local"
  58   │     }
  59   │   ]
  60   │ }

Easter egg: I don't think 'pull' actually exists. It's 'push' - I was trying to get in some entropy for lorri.

blaggacao · 2020-07-20T22:17:27Z

Merge, 2, 3?
1, 2, merge?

😉 Thank you @roberth for your guidance!

Follow up to cachix#52

blaggacao · 2020-07-21T00:09:43Z

pre-commit uninstalled
pre-commit installed at .git/hooks/pre-push  # sic! pre-push

... now let's call it working!

The code before the last commit only ever installed 'commit' stage.

The code after the commit installs all known stages if they are configured or falls back to 'commit'.
It also removes old, eventually outdated stages.

modules/pre-commit.nix

roberth

Just some simplifications around installation

modules/pre-commit.nix

blaggacao · 2020-07-21T17:38:55Z

@roberth
I'd like to apologize for my imperative opinions. However, without controlling the side effects, my use case gets completely jeopardized.

I'd suggest for the intermediate term to adopt a work around with a more involved error catcher.

Let's track pre-commit/pre-commit#1541 in complement to this course of action in a separate issue.

Would that be in line with your concerns?

blaggacao · 2020-07-21T20:12:17Z

We maybe, more fundamentally, have to ask ourselves, what stance to take on the pre-commit author's intentions:

pre-commit intentionally makes opting in and out an explicit action

Since in the current implementation pre-commit install (with or without this PR) is triggered, this would violate the author's intention.

On the other hand, shell.nix holds promises about repeatable development environments.
Opting out is still (somewhat) possible in this scenario until the declarative configuration updates.

EDIT: @domenkozar @roberth What do you think of out-factoring the install phase into another optional option, so we more faithfully maintain the opt-in + opt-out nature of pre-commit? - I don't have any better idea at the moment.

domenkozar · 2020-07-22T07:51:24Z

Since the tool is called pre-commit I would expect the default to be running hooks before committing.

blaggacao · 2020-07-22T17:04:30Z

Since the tool is called pre-commit I would expect the default to be running hooks before committing.

Fair enough 😄

In reality though, pre-commit is evolving into a repo-scoped (per-repo cache) packager for task runners of all sorts.

To that end, I really like that you cut this sprout and called it local 👍

So once we strip that we are left with — despite the name — a git hook manager similar to husky.

Husky, contrary to pre-commit does make version controlled pretentions about which hooks to be installed.

See also:

npm uninstall husky
Git hooks installed by husky will be removed.

Contrary to pre-commit, it also cleans up after itself, by default.

I like husky's approach better and I think we should take the liberty and "fix" pre-commit for the nix community.

I can assure you, pre-commit's model is not useful as-is for me who has to make sure my collaborator is always up to date with the latest coding guidelines. That's why I'm turning to nix altogether in the first place.

roberth · 2020-07-22T17:21:29Z

At first glance, husky doesn't seem to provide help with the optimization of running only on modified files. Also iirc some users of this project also use or want to use existing non-Nixified pre-commit "repos" (packaged via other language-specific means)

I don't think it would be beneficial to kick out pre-commit in favor of another tool for these reasons.
The issue this PR is facing is minor. The lack of a pre-commit uninstall --all can be worked around, as you have done. Sure that means maintaining a list of hooks, but that's far better than fragmenting the Nix community's git hook packaging effort.

blaggacao · 2020-07-22T19:26:18Z

@roberth Totally agree with both apsects (keeping pre-commit and working around its limitations).

Thanks for your opinion!

Counting your vote, my implicit vote (via this PR) and @domenkozar apparent abstention for a clear vote (please correct), I might summarize that we agree moving forward with what this PR tries to attempt.

I have already deployed it in our organization and did a last fixup yesterday.

Therefore, may I suggest you proceed with a formal incremental review?

blaggacao · 2020-07-22T19:27:45Z

Oh, I'll have a look at the tests after lunch.

nix/sources.json

blaggacao · 2020-07-29T02:10:25Z

Shall we proceed?

blaggacao · 2020-08-07T01:12:39Z

Ahm... From the CI.

 error: attribute 'Cabal_3_0_0_0' missing, at /home/runner/work/pre-commit-hooks.nix/pre-commit-hooks.nix/nix/default.nix:20:27
(use '--show-trace' to show detailed location information)
##[error]Process completed with exit code 1.

I hope that's not because of my latest commit, if so, I'd ask you to solve this the right way so I can learn by example.

blaggacao · 2020-09-02T20:03:50Z

@domenkozar What's holding this back? Is it time?

domenkozar · 2020-09-03T07:38:01Z

Bumping nixpkgs to master broke a few things, so we probably best revert that and have two versions of nixpkgs for tooling. I think eventually we'll need that sooner than later.

Co-authored-by: Robert Hensing <[email protected]>

This reverts commit bf4b11f. The latest nixpkgs version still breakes things.

blaggacao · 2020-09-03T20:58:39Z

@domenkozar Ok I reverted it but I do unfortunately not know how to enable two nixpkgs versions.

Since on my side this is at sharp risk of getting stale, and I'm not able to give it the final touch, might I ask for your help to finish this off?

domenkozar · 2020-09-07T13:59:06Z

Since I don't have time for this I recommend we wait for 20.10 to come out and pin that one which should also unblock this merge.

domenkozar · 2020-09-17T14:17:21Z

I've bump nixpkgs in #65 and this issue is a good reminder that #61 is quite error prone.

domenkozar · 2020-10-06T17:58:36Z

Alright master is ready!

blaggacao · 2020-11-02T03:18:08Z

replaced by #79

blaggacao force-pushed the master branch from 79f59c2 to f4671e1 Compare July 20, 2020 21:40

blaggacao force-pushed the master branch from f4671e1 to ba601e7 Compare July 20, 2020 21:51

blaggacao commented Jul 20, 2020

View reviewed changes

modules/pre-commit.nix Show resolved Hide resolved

blaggacao force-pushed the master branch 2 times, most recently from cf5087d to dc6efb8 Compare July 20, 2020 22:06

blaggacao pushed a commit to blaggacao/pre-commit-hooks.nix that referenced this pull request Jul 20, 2020

Suggest default_stages

5ee71d6

Follow up to cachix#52

blaggacao mentioned this pull request Jul 20, 2020

Suggest default_stages #53

Merged

blaggacao force-pushed the master branch from 1149d5f to 795e813 Compare July 21, 2020 00:07

blaggacao commented Jul 21, 2020

View reviewed changes

modules/pre-commit.nix Outdated Show resolved Hide resolved

roberth reviewed Jul 21, 2020

View reviewed changes

modules/pre-commit.nix Outdated Show resolved Hide resolved

modules/pre-commit.nix Outdated Show resolved Hide resolved

modules/pre-commit.nix Show resolved Hide resolved

modules/pre-commit.nix Outdated Show resolved Hide resolved

blaggacao force-pushed the master branch from b0940e6 to ac0ce1e Compare July 21, 2020 18:03

blaggacao mentioned this pull request Jul 21, 2020

make supported hooks cli-discoverable pre-commit/pre-commit#1541

Closed

blaggacao force-pushed the master branch 3 times, most recently from 36f7093 to 3fa3a17 Compare July 21, 2020 20:01

blaggacao force-pushed the master branch from 3fa3a17 to 3b02665 Compare July 21, 2020 21:55

domenkozar reviewed Jul 24, 2020

View reviewed changes

nix/sources.json Outdated Show resolved Hide resolved

blaggacao requested a review from domenkozar August 7, 2020 01:05

jiribenes added a commit to jiribenes/pre-commit-hooks.nix that referenced this pull request Aug 13, 2020

Merge PR cachix#52 from upstream

190e228

David Arnold and others added 11 commits September 3, 2020 15:02

feat: support specifying stages, close cachix#49

90a70cb

fix: add intialization according to hook type

372bb48

imp: work around the possibility of different pre-commit versions

efc5b3e

pin pre-commit to v2.6.0 for 'post-' hooks support

087925f

fix: never fail silently

81e8853

fix: leave the future self with a hint

76d8c34

typo

cc2462a

Co-authored-by: Robert Hensing <[email protected]>

niv: fix version tor tag

29eed70

fix: hint possible future per-hook stage implementation

8afbe87

nixpkgs: pin to unstable and unpin pre-commit

c2ef418

Revert "nixpkgs: pin to unstable and unpin pre-commit"

a8e8868

This reverts commit bf4b11f. The latest nixpkgs version still breakes things.

blaggacao force-pushed the master branch from bf4b11f to a8e8868 Compare September 3, 2020 20:05

fix: remove pre-commit from niv sources (no effect)

5a58e5c

blaggacao force-pushed the master branch from b3bd176 to 5a58e5c Compare September 3, 2020 21:01

blaggacao mentioned this pull request Sep 4, 2020

Make usable without package distribution #61

Open

4 tasks

blaggacao mentioned this pull request Sep 26, 2020

DevShell native pre commit hooks numtide/devshell#16

Closed

blaggacao mentioned this pull request Nov 2, 2020

feat: support specifying stages, close #49 #79

Merged

blaggacao closed this Nov 2, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: support specifying stages #52

feat: support specifying stages #52

blaggacao commented Jul 20, 2020 •

edited

Loading

roberth commented Jul 20, 2020

roberth commented Jul 20, 2020

blaggacao commented Jul 20, 2020 •

edited

Loading

roberth commented Jul 20, 2020

blaggacao commented Jul 20, 2020 •

edited

Loading

blaggacao commented Jul 20, 2020

blaggacao commented Jul 21, 2020 •

edited

Loading

roberth left a comment

blaggacao commented Jul 21, 2020 •

edited

Loading

blaggacao commented Jul 21, 2020 •

edited

Loading

domenkozar commented Jul 22, 2020

blaggacao commented Jul 22, 2020 •

edited

Loading

roberth commented Jul 22, 2020

blaggacao commented Jul 22, 2020

blaggacao commented Jul 22, 2020

blaggacao commented Jul 29, 2020

blaggacao commented Aug 7, 2020

blaggacao commented Sep 2, 2020

domenkozar commented Sep 3, 2020

blaggacao commented Sep 3, 2020 •

edited

Loading

domenkozar commented Sep 7, 2020

domenkozar commented Sep 17, 2020

domenkozar commented Oct 6, 2020

blaggacao commented Nov 2, 2020

feat: support specifying stages #52

feat: support specifying stages #52

Conversation

blaggacao commented Jul 20, 2020 • edited Loading

roberth commented Jul 20, 2020

roberth commented Jul 20, 2020

blaggacao commented Jul 20, 2020 • edited Loading

roberth commented Jul 20, 2020

blaggacao commented Jul 20, 2020 • edited Loading

blaggacao commented Jul 20, 2020

blaggacao commented Jul 21, 2020 • edited Loading

roberth left a comment

Choose a reason for hiding this comment

blaggacao commented Jul 21, 2020 • edited Loading

blaggacao commented Jul 21, 2020 • edited Loading

domenkozar commented Jul 22, 2020

blaggacao commented Jul 22, 2020 • edited Loading

roberth commented Jul 22, 2020

blaggacao commented Jul 22, 2020

blaggacao commented Jul 22, 2020

blaggacao commented Jul 29, 2020

blaggacao commented Aug 7, 2020

blaggacao commented Sep 2, 2020

domenkozar commented Sep 3, 2020

blaggacao commented Sep 3, 2020 • edited Loading

domenkozar commented Sep 7, 2020

domenkozar commented Sep 17, 2020

domenkozar commented Oct 6, 2020

blaggacao commented Nov 2, 2020

blaggacao commented Jul 20, 2020 •

edited

Loading

blaggacao commented Jul 20, 2020 •

edited

Loading

blaggacao commented Jul 20, 2020 •

edited

Loading

blaggacao commented Jul 21, 2020 •

edited

Loading

blaggacao commented Jul 21, 2020 •

edited

Loading

blaggacao commented Jul 21, 2020 •

edited

Loading

blaggacao commented Jul 22, 2020 •

edited

Loading

blaggacao commented Sep 3, 2020 •

edited

Loading