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

refactor!: add Makefile rule for formatting using ocamlformat #407

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

EmileRolley
Copy link
Collaborator

@EmileRolley EmileRolley commented Jul 28, 2021

In order to have a consistent formatted source code, this PR adds a Makefile rule format calling ocamlformat.0.8 and provides a minimal .ocamlformat file (which is not meant to be definitive at all).

Note: learn-ocaml using dune.1.7, the stanza formatting can't be used. That's why I added the rule in the Makefile.

@EmileRolley EmileRolley force-pushed the setup-ocamlformat branch 2 times, most recently from 21578d4 to a32d47c Compare July 28, 2021 10:53
@EmileRolley EmileRolley added the kind: enhancement Enhancement to an existing user-facing feature. label Jul 28, 2021
@EmileRolley EmileRolley requested a review from yurug July 28, 2021 10:54
@EmileRolley EmileRolley force-pushed the setup-ocamlformat branch 2 times, most recently from 15dd6ab to a26f3d4 Compare July 28, 2021 11:07
@AltGr
Copy link
Collaborator

AltGr commented Jul 28, 2021

I think ocamlformat is fine for new projects, but in this case I wonder if it is worth it to break all of the git history (by which I mean git blame-related functions)... Apparently it's not so common but I check the git history of a given source line to understand why it was put there pretty often.

@erikmd
Copy link
Member

erikmd commented Jul 28, 2021

Agree with @AltGr; moreover, merging this ocamlformat change at some point would mean that all currently opened PRs will raise git conflicts everywhere… so if one decide to fully switch to ocamlformat, one should at least wait for having no open PR 😅

otherwise, ideally (but I don't know if this is possible), ocamlformat could only rewrite code lines that have been recently edited… (using git diff info or so)

@EmileRolley
Copy link
Collaborator Author

I think ocamlformat is fine for new projects, but in this case I wonder if it is worth it to break all of the git history (by which I mean git blame-related functions)... Apparently it's not so common but I check the git history of a given source line to understand why it was put there pretty often.

Since git 2.23, using the --ignore-rev flag can specify a commit to be ignored by git blame. I added the commit hash in the git-blame-ignore-revs file.
So, you just need to run once:

git config blame.ignoreRevsFile .git-blame-ignore-revs 

and git blame will ignore changes caused by formatting.

@EmileRolley
Copy link
Collaborator Author

[...] merging this ocamlformat change at some point would mean that all currently opened PRs will raise git conflicts everywhere...

I think for this point, updating the PR branch from the master one and then run make format should be enough, no?

@EmileRolley EmileRolley force-pushed the setup-ocamlformat branch 2 times, most recently from e8fc918 to 86dbad7 Compare July 28, 2021 16:42
@erikmd
Copy link
Member

erikmd commented Jul 28, 2021

[...] merging this ocamlformat change at some point would mean that all currently opened PRs will raise git conflicts everywhere...

I think for this point, updating the PR branch

How should we do this?

IINM, either git fetch upstream && git merge upstream/master or git fetch upstream && git rebase upstream/master would create a ton of conflicts (one for each line that was both modified in the branch by the user / in master by ocamlformat)

@EmileRolley
Copy link
Collaborator Author

You are certainly right... What about installing ocamlformat and formatting the source code manually in branches before merging master into them?

@erikmd
Copy link
Member

erikmd commented Aug 25, 2021

Sorry, forgot to reply earlier in this thread after we discussed this a bit with @yurug last month 😅

Trying to sum up the ideas:

  • it will be a quality-of-life PR for new contributions so we'll be happy to merge this at some point;

  • but as mentioned above, it is certainly too early to merge it now… so let's say, probably for learn-ocaml 1.0.0 :-)

as a result, I'll mark this PR in draft mode now, so we don't accidentally merge it.

@erikmd erikmd marked this pull request as draft August 25, 2021 22:28
@EmileRolley
Copy link
Collaborator Author

Hi @erikmd,

Sorry, forgot to reply earlier in this thread after we discussed this a bit with @yurug last month

No problem :)

but as mentioned above, it is certainly too early to merge it now… so let's say, probably for learn-ocaml 1.0.0 :-)

I don't really get how waiting will make easier the transition? Indeed, there will always be one open PR that will delay it, no?
I seem to have heard in one of Yann's classes something like: "fix broken windows as soon as possible". However, it is not a broken window so I guess it could be done later 😇

@erikmd
Copy link
Member

erikmd commented Aug 26, 2021

Hi @EmileRolley,

I don't really get how waiting will make easier the transition? Indeed, there will always be one open PR that will delay it, no?

Sure! But it happens there are quite a few PRs open now, and most of them are not "small"…

(admittedly, it's better if PRs are small! to ease review and so on; so, maybe, better next time ;-)

@erikmd erikmd changed the title feat!: add Makefile rule for formatting refactor!: add Makefile rule for formatting Oct 16, 2021
@erikmd erikmd changed the title refactor!: add Makefile rule for formatting refactor!: add Makefile rule for formatting using ocamlformat Oct 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind: enhancement Enhancement to an existing user-facing feature.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants