Skip to content

Conversation

@exarkun
Copy link
Contributor

@exarkun exarkun commented Jan 11, 2023

I'm not sure about this. I added a fourmolu configuration but I don't know if we can make the fourmolu pre-commit hook respect it.

@meejah
Copy link

meejah commented Jan 11, 2023

I don't have enough Haskell experience to have good opinions on "proper formatting" but what do you mean about the pre-commit hook thing? (I don't see any hooks set up / edited in this PR).

I strongly prefer a pre-push hook in general: I often commit things that don't fully work and getting slowed down by "reformat it right now" sucks. Usually the important piece here is to not put "un-formatted" code into a PR (so that's why I like pre-push for these; remind you if your code isn't formatted when pushing).

That said, "in general" automated formatting is good so the style is consistent, so +1 from me there :)

@exarkun
Copy link
Contributor Author

exarkun commented Jan 11, 2023

I don't have enough Haskell experience to have good opinions on "proper formatting"

Yea, neither do I, I think. But I guess anything that fourmolu spits out is probably going to be better than a free for all.

but what do you mean about the pre-commit hook thing? (I don't see any hooks set up / edited in this PR).

Indeed. The hooks can be set up by the flake added in #32 but there is none in master yet. These changes I got by running fourmolu by hand.

I strongly prefer a pre-push hook in general: I often commit things that don't fully work and getting slowed down by "reformat it right now" sucks. Usually the important piece here is to not put "un-formatted" code into a PR (so that's why I like pre-push for these; remind you if your code isn't formatted when pushing).

Yea, that workflow makes sense to me as well. The pre-commit thing that #32 pulls in lets us make the hooks into a pre-push hook if we want - and I'd be on-board with doing that.

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.

2 participants