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

Enable ocamlformat #914

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

Enable ocamlformat #914

wants to merge 2 commits into from

Conversation

smorimoto
Copy link
Member

No description provided.

Signed-off-by: Sora Morimoto <[email protected]>
Signed-off-by: Sora Morimoto <[email protected]>
@MisterDA
Copy link
Contributor

I'm in favour of using ocamlformat, for consistency and readability.
You left out the files using cppo (e.g. lwt_unix.cppo.ml) because ocamlformat doesn't support files using preprocessing, right? Is there a way we could cheat and add a formatted version of these files to the PR?
To ease git-blaming, I also suggest that we add a .git-blame-ignore-revs file, containing the commit id of the re-formatting. The file can then be used with git blame --ignore-revs-file .git-blame-ignore-revs. See some blog posts explaining the idea:
Ignoring mass reformatting commits with git blame
, Exclude linting & formatting commits when running Git blame.

src/core/lwt.ml Outdated
callbacks.cleanups_deferred <- cleanups_deferred)
ps
|> List.iter (fun p ->
let (Internal p) = to_internal_promise p in
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is not a helpful comment, but I'm sad about this. Unlike most code, lwt.ml is carefully formatted to look good, and ocamlformat is introducing things such as this indentation.

Copy link
Collaborator

Choose a reason for hiding this comment

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

To make it potentially somewhat helpful, in the past we applied ocp-indent (then the go-to formatting tool) to the almost the whole repo, but we avoided touching code that had gotten the full human attention.

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.

3 participants