learn-ocaml icon indicating copy to clipboard operation
learn-ocaml copied to clipboard

refactor!: add Makefile rule for formatting using ocamlformat

Open EmileRolley opened this issue 4 years ago • 9 comments

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 avatar Jul 28 '21 10:07 EmileRolley

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.

AltGr avatar Jul 28 '21 11:07 AltGr

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)

erikmd avatar Jul 28 '21 13:07 erikmd

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 avatar Jul 28 '21 13:07 EmileRolley

[...] 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 avatar Jul 28 '21 13:07 EmileRolley

[...] 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)

erikmd avatar Jul 28 '21 17:07 erikmd

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

EmileRolley avatar Jul 28 '21 17:07 EmileRolley

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 avatar Aug 25 '21 22:08 erikmd

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 :innocent:

EmileRolley avatar Aug 26 '21 08:08 EmileRolley

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 avatar Aug 26 '21 21:08 erikmd