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

Use the module system #19

Merged
merged 7 commits into from
Oct 2, 2019
Merged

Use the module system #19

merged 7 commits into from
Oct 2, 2019

Conversation

roberth
Copy link
Contributor

@roberth roberth commented Sep 25, 2019

Closes #2
Closes #17

TODO

  • expose and test "core" module set from project.nix
  • niv update -b master project.nix when that's done (Pre commit hercules-ci/project.nix#10)
  • changelog entry: (elaborate on) translate .pre-commit-config.yaml to hooks argument
  • update README
  • add how to add a hook to README

New

Fixes

  • Stop installing files all over the place when doing nix-shell ../some-project to borrow a tool
  • Lorri support

Cleanup

  • Get rid of some installation code

New
 - Selectively enable tools (#2)
 - Make choice of tool versions overridable
 - User-definable hooks
 - Input validation at the Nix level

Fixes
 - Stop installing files all over the place when doing nix-shell ../some-project to borrow a tool
 - Lorri support

Cleanup
 - Get rid of installation code
@roberth roberth requested a review from domenkozar September 25, 2019 22:48
Copy link
Member

@domenkozar domenkozar left a comment

Choose a reason for hiding this comment

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

There are still references to project.nix, which are not used.

Looks good! Wish there were some docs on how to override hooks or add new ones, but we can add that later.

README.md Show resolved Hide resolved
@roberth
Copy link
Contributor Author

roberth commented Sep 29, 2019

There are still references to project.nix, which are not used.

It's used by the shellHook for the auto-installation part. With "delegate" in the PR title I meant pulling it in from there and maintaining it there. Removing shellHook entirely and directing people to project.nix for this functionality seems a bit too soon.
So do we want this project to be usable on its own? If so, does it matter whether we pin or duplicate?

@domenkozar
Copy link
Member

domenkozar commented Sep 29, 2019

Removing shellHook entirely and directing people to project.nix for this functionality seems a bit too soon.

Why can't we just expose shellHook as it was before this PR and project.nix can pick it up?

Maybe there should be nix/project.nix with:

{ ... }: {
  config.activation.hooks = (import ../default.nix { }).shellHook;
}

Which is later going to be translated into flakes.

Once we need to merge project.nix options we can do that.

@roberth
Copy link
Contributor Author

roberth commented Sep 29, 2019

Why can't we just expose shellHook as it was before this PR

This PR does not change that part of the nix-pre-commit API, so it is exposed as before, but with an implementation that is not directly inside the repo. The project.nix implementation fixes some problems and has improvements over the current shellHook implementation. Of course these can be backported but that seems a bit pointless to me.

and project.nix can pick it up?

This is can be done, but it must be an acceptable hook for the activation module, rather than a standalone autoinstall script. It's not a big difference, just have to expose two things instead of one.

@roberth roberth changed the title Use the module system, delegate activation to project.nix core Use the module system Oct 1, 2019
@roberth roberth requested a review from domenkozar October 2, 2019 08:18
Copy link
Member

@domenkozar domenkozar left a comment

Choose a reason for hiding this comment

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

Looks good, major immediate win is that not all tooling is downloaded, so tooling can now grow.

@roberth
Copy link
Contributor Author

roberth commented Oct 2, 2019

bors r+

bors bot added a commit that referenced this pull request Oct 2, 2019
19: Use the module system r=roberth a=roberth

Closes #2
Closes #17 

TODO
 - [x] ~expose and test "core" module set from project.nix~
 - [x] ~niv update -b master project.nix when that's done (hercules-ci/project.nix#10
 - [x] changelog entry: (elaborate on) translate `.pre-commit-config.yaml` to hooks argument
 - [x] update README
 - [x] add how to add a hook to README

New
 - Selectively enable tools (#2)
 - Make choice of tool versions overridable
 - User-definable hooks
 - Input validation at the Nix level
 - Use gitignore by default

Fixes
 - Stop installing files all over the place when doing nix-shell ../some-project to borrow a tool
 - Lorri support

Cleanup
 - Get rid of some installation code

Co-authored-by: Robert Hensing <[email protected]>
@bors
Copy link
Contributor

bors bot commented Oct 2, 2019

Build failed

@roberth
Copy link
Contributor Author

roberth commented Oct 2, 2019

bors is broken?
Merging as admin...

Anyway 🎉

@roberth roberth merged commit f823ad3 into master Oct 2, 2019
@bors bors bot deleted the modular branch October 2, 2019 08:37
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.

Add docs, examples for overriding provided hooks
2 participants