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

Improve and fix hpack hook #236

Merged
merged 2 commits into from
Feb 2, 2023
Merged

Improve and fix hpack hook #236

merged 2 commits into from
Feb 2, 2023

Conversation

Niols
Copy link
Contributor

@Niols Niols commented Jan 30, 2023

closes #235

Following #235, here is a PR that improves and fixes the hpack hook.

Changes

...to the files pattern

First, we update the files pattern to match *.cabal and package.yaml files everywhere in the repository and not at the root. The files pattern's components change in the following way:

  • The first part, \\.l?hs(-boot)?$, remains the same. To be honest, I do not understand why this part is useful or necessary. But I did not take the liberty to remove it.
  • The second part, ^[^/]+\\.cabal$, changes to \\.cabal$. This makes it match any *.cabal file in the repository, while the previous version matched only *.cabal files at top-level.
  • The third part, ^package\\.yaml$, changes to (^|/)package\\.yaml$. Once again, this makes it match any package.yaml file in the repository while the previous version matched them only at top-level.

The files pattern matches on the files's paths and not just the names. I did not find any direct reference to that in the documentation of pre-commit. However, its section “Filtering files with types” mentions the following examples:
image

...to the hpack-dir script

Second, we rewrite the hpack-dir script to achieve two things:

  1. We replace the call to hpack by a call to ${hpack}/bin/hpack. This ensures that the script works also when hpack is not present on the machine. This also increases reliability by giving better control to hpack's version being used in the hook.

  2. The previous version also failed silently in case of errors of hpack (or error caused by the absence of hpack). We suggest a replacement based on set -e and find | while. This design choice is heavily documented in the script itself.

Experiments

I have introduced this “fixed” version of the hook in two of Tweag's projects: tweag/pirouette#177 and tweag/cooked-validators#226. I ran some tests locally by hand on both those projects, and also in CI; all have proven successful. The first project has its package.yaml and *.cabal project at top-level and the second within sub-directories.

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.

The hpack hook seems broken in several different ways
2 participants