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

Jump to bottom

Correct issues with pre-commit hook #2597

Merged
merged 10 commits into from
Jan 24, 2022
Merged

Correct issues with pre-commit hook #2597

merged 10 commits into from
Jan 24, 2022

Conversation

bradrn
Copy link
Contributor

@bradrn bradrn commented Jan 17, 2022 •
edited
Loading

Following the Contributing guidelines, I added the specified pre-commit hook when I cloned HLS. However testing it revealed several issues, which are fixed in this PR.

bradrn added 4 commits January 17, 2022 15:16
@bradrn
Reformat in accordance with pre-commit hook
800c165
@bradrn
Typo fix: add missing comma to documented pre-commit hook
26c5821
@bradrn
Exclude failing files from pre-commit hook
86de39c 
'stylish-haskell' seems to always fail with a parse error on several
files under ghcide/. Excluding these files from the pre-commit hook
ensures that this hook can always succeed.
@bradrn
Merge branch 'master' of https://github.com/haskell/haskell-language-…
c6fcdc3 
…server
@jneira
Copy link
Member

jneira commented Jan 17, 2022

Hi, thanks for fixing the precommit hook. This include the formatting of some files inside ghcide and i am not sure if it is on purpose.

@jneira jneira requested a review from pepeiborra January 17, 2022 07:23
@bradrn
Copy link
Contributor Author

bradrn commented Jan 17, 2022

This include the formatting of some files inside ghcide and i am not sure if it is on purpose.

Yes, 800c165 was on purpose. My goal was to ensure that the pre-commit hook would be a no-op for freshly pulled code — only modified code would be reformatted.

@jneira
Copy link
Member

jneira commented Jan 17, 2022

I am ok with the reformat code and there is no pr touching them now afaics, but would like to know @pepeiborra thoughts

@pepeiborra
Copy link
Collaborator

What were the issues with the hook?

@bradrn
Copy link
Contributor Author

bradrn commented Jan 22, 2022

What were the issues with the hook?

It had a syntax error (fixed in 26c5821), it wasn’t the identity on freshly pulled source code (fixed in 800c165), and it failed when running on certain files (fixed in 86de39c).

@pepeiborra
Copy link
Collaborator

pepeiborra commented Jan 23, 2022 •
edited
Loading

What were the issues with the hook?

It had a syntax error (fixed in 26c5821), it wasn’t the identity on freshly pulled source code (fixed in 800c165), and it failed when running on certain files (fixed in 86de39c).

Thanks!

Could you also update the Nix version of the hook with the newly excluded files?

haskell-language-server/flake.nix

Lines 171 to 175 in ff0ccd6

"^ghcide/src/Development/IDE/GHC/Compat.hs$"
"^plugins/hls-splice-plugin/src/Ide/Plugin/Splice.hs$"
"^ghcide/test/exe/Main.hs$"
"ghcide/src/Development/IDE/Core/Rules.hs"
"^hls-test-utils/src/Test/Hls/Util.hs$"

@bradrn
Copy link
Contributor Author

bradrn commented Jan 23, 2022

Could you also update the Nix version of the hook with the newly excluded files?

Done! (I don’t use Nix regularly, so didn’t even notice there was a Nix version…)

pepeiborra
pepeiborra approved these changes Jan 23, 2022
View reviewed changes
Copy link
Collaborator

@pepeiborra pepeiborra left a comment

Choose a reason for hiding this comment

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

Thank you!

@Ailrun Ailrun added the merge me Label to trigger pull request merge label Jan 23, 2022
Ailrun
Ailrun approved these changes Jan 23, 2022
View reviewed changes
Copy link
Member

@Ailrun Ailrun left a comment

Choose a reason for hiding this comment

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

LGTM too. Could you resolve the conflict?

bradrn added 2 commits January 24, 2022 10:08
@bradrn
Merge remote-tracking branch 'upstream/master'
95a1d34
@bradrn
Reformat again in accordance with pre-commit hook
806e9b6 
This is basically the same as 800c165, but applied to new changes
which have been added since then.
@mergify mergify bot merged commit f33954e into haskell:master Jan 24, 2022
@Ailrun Ailrun mentioned this pull request Jan 24, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Ailrun Ailrun Ailrun approved these changes

@pepeiborra pepeiborra pepeiborra approved these changes

Assignees
No one assigned
Labels
merge me Label to trigger pull request merge
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@bradrn @jneira @pepeiborra @Ailrun