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

Documentation.md: add pre-commit hook sample #1392

Merged
merged 3 commits into from
Jan 27, 2021
Merged

Conversation

knocte
Copy link
Contributor

@knocte knocte commented Jan 25, 2021

@knocte
Copy link
Contributor Author

knocte commented Jan 26, 2021

@nojaf FYI I tested this on macOS (which should mean it also works on Linux) and @aarani tested on Windows.

@nojaf
Copy link
Contributor

nojaf commented Jan 26, 2021

Hello Andres, thanks for writing this down.
I've added a few extra lines to manages people's expectations.
This can work wonderfully or horribly depending on what you do in your code.
Was I right to assume that you've installed the tool globally?

@knocte
Copy link
Contributor Author

knocte commented Jan 27, 2021

This can work wonderfully or horribly depending on what you do in your code.

What do you mean?

Was I right to assume that you've installed the tool globally?

Yes indeed, but somehow I had to add the absolute path to fantomas, otherwise the hook would fail :(

@nojaf
Copy link
Contributor

nojaf commented Jan 27, 2021

What do you mean?

Formatting before committing. Fantomas could mess up your code right before you commit.
So using this hook could be wonderful or horrible.
We are all working hard to improve this but I feel like we should still add a fair warning.

@knocte
Copy link
Contributor Author

knocte commented Jan 27, 2021

but I feel like we should still add a fair warning.

Ah yes sure. Anyway if fantomas fails to format, thanks to #1376 it would abort the hook AFAIU.

@nojaf
Copy link
Contributor

nojaf commented Jan 27, 2021

Well, that is the tricky part. Fantomas does not know what your code is doing. So formatting may have worked but it could change the semantics of the code. In any case, I think we can always add more warnings later if necessary.

@nojaf nojaf enabled auto-merge (squash) January 27, 2021 07:29
@nojaf nojaf merged commit 138146e into fsprojects:master Jan 27, 2021
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.

Format in pre-commit hook
2 participants