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

Add pre-commit hook for cleaning up mixed-line endings #2679

Merged
merged 12 commits into from
Feb 4, 2022

Conversation

drsooch
Copy link
Collaborator

@drsooch drsooch commented Feb 2, 2022

Figured I'd offer a solution for #2678. I don't use nix so I'm not sure if there is anything that needs to be done to integrate in nix (also not sure if it's even necessary). Most of the file commits are the line endings with a few interspersed stylish-haskell changes.

@michaelpj
Copy link
Collaborator

Hmm, isn't there a git setting that sorts this out for you? Something with git config core.autocrlf?

@jneira
Copy link
Member

jneira commented Feb 2, 2022

Hmm, isn't there a git setting that sorts this out for you? Something with git config core.autocrlf?

yeah core.autocrlf=input to ensure you use LF endings (the used ones in the remote repo) but:

So maybe add a precommit hook to enforce it is not a bad idea

@@ -1,162 +1,162 @@
{-# LANGUAGE DerivingVia #-}
Copy link
Member

Choose a reason for hiding this comment

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

omg all those files had incorrect line endings???? 🤦

Copy link
Member

Choose a reason for hiding this comment

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

yeah, as described in the issue: #2678

"args": ["--fix", "lf"],
"exclude": "test/testdata/.*CRLF*.hs$"
}
]
Copy link
Member

@jneira jneira Feb 2, 2022

Choose a reason for hiding this comment

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

what about create a .pre-commit-config.json in the repo to make a little bit more easier and discoverable? docs could link to the file in that case

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I can do that as well!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Although, the pre-commit hook is part of .gitignore. I can envision some users use their own personal pre-commit hook and having one in source control may cause issues?

@jneira
Copy link
Member

jneira commented Feb 2, 2022

So maybe add a precommit hook to enforce it is not a bad idea

otoh you have to remember install the hook, like you have to remember install the plugin to honour our existing enforcing mechanism, .editorconfig 🤷

the advantage of the hook is it is applied always for sure (once is installed), even if you editor (notepad?? :-P) does not have support for .editorconfig

@jneira jneira linked an issue Feb 2, 2022 that may be closed by this pull request
@jneira
Copy link
Member

jneira commented Feb 2, 2022

What about fix the formatting existing hook to make it use LF in its formatting (or at least not change them to CRLF!!!)? we would have the two features with one hook

@drsooch
Copy link
Collaborator Author

drsooch commented Feb 3, 2022

What about fix the formatting existing hook to make it use LF in its formatting (or at least not change them to CRLF!!!)? we would have the two features with one hook

Ooo I didn't realize stylish-haskell could fix line-endings. I guess the only downside is it won't get run on a bunch of excluded files?

@Anton-Latukha
Copy link
Collaborator

Special thanks for converting those files to the initial line ending. Converting back - would solve Git diff rebases. I thought that is the first thing to do on return, before thinking further, it is already done here. Thank you.

@jneira
Copy link
Member

jneira commented Feb 3, 2022

What about fix the formatting existing hook to make it use LF in its formatting (or at least not change them to CRLF!!!)? we would have the two features with one hook

Ooo I didn't realize stylish-haskell could fix line-endings. I guess the only downside is it won't get run on a bunch of excluded files?

not sure if it could modify line endings but it should not change the existing ones, like it is doing for me lf -> crlf
that alone would be an improvement

@@ -32,7 +32,7 @@ test/testdata/**/hie.yaml
.shake/

# pre-commit-hook.nix
.pre-commit-config.yaml
#.pre-commit-config.yaml
Copy link
Member

Choose a reason for hiding this comment

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

Is there a way to put the config into nix setting?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't use nix so I have no idea, any help would be appreciative.

Copy link
Member

Choose a reason for hiding this comment

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

nix doesn't support windows so I suppose it is not necessary?

Copy link
Member

Choose a reason for hiding this comment

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

Oh, makes sense.

Copy link
Member

@jneira jneira left a comment

Choose a reason for hiding this comment

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

thanks!

@Anton-Latukha Anton-Latukha added the merge me Label to trigger pull request merge label Feb 4, 2022
@mergify mergify bot merged commit 411db02 into haskell:master Feb 4, 2022
@drsooch drsooch deleted the pre-commit-mixed-line-endings branch August 20, 2022 02:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge me Label to trigger pull request merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

automation: Please, check/rewrite carrige return
5 participants