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

consistent project-wide formatting #155

Merged
merged 4 commits into from
Mar 15, 2022

Conversation

glennsl
Copy link
Contributor

@glennsl glennsl commented Mar 6, 2022

This started with me wanting to pin ocamlformat to a specific version because #154 failed CI despite not failing locally because of a new version introducing a different. Then I went on to unify all the .ocamlformat files into one project-wide configuration, and finally added some tweaks to it that I've found to work well, because I found the code I added in #154 got absolutely butchered compared to the version I have in my own project.

For comparison:

let use_effect ~on f =
  let last_value = React.use_ref on in

  (* runs initially *)
  React.use_effect0 (fun () ->
      f ();
      None);

  (* runs always *)
  React.use_effect (fun () ->
      if on <> React.Ref.current last_value then begin
        React.Ref.set_current last_value on;
        f ()
      end;
      None)
let use_effect ~on f =
  let last_value = Core.use_ref on in
  Core.use_effect_once (fun () -> f () ; None) ;
  Core.use_effect_always (fun () ->
      if on <> Core.Ref.current last_value then (
        Core.Ref.set_current last_value on ;
        f () ) ;
      None )

The reasoning for the configuration choices is as follows:

  • profile = default - I find both default and ocamlformat to be pretty inconsistent, ocamlformat more so than default, and actually prefer janestreet I think, if not for one notable exception: that it doesn't preserve single blank lines in function definitions, which I find absolutely essential for grouping. And there's unfortunately no configuration option for that. Therefore default seems like the more sensible baseline.
  • break-separators = before and dock-collection-brackets = false - These put the delimiters and separators of lists, records etc. in front of the items, and in the same column, i.e. Haskell/Elm-style. This makes line-based editing easier while also being more compact, and arguably more readable (once familiar with the style). The downside is that it's quite unfamiliar to people coming from JavaScript and similar languages.
  • exp-grouping = preserve - Simply enables the use of begin/end instead of parentheses, in rare cases where that's more readable.
  • type-decl = sparse - Always formats variants and records with each item on a separate line instead of trying to fit them on a single line. This is more consistent, easier to read, and makes line-based editing much easier.
  • break-fun-decl = fit-or-vertical and break-fun-sig = fit-or-vertical - Puts function arguments on separate lines if they can't all fit on a single line, instead of trying to fit as much as possible on each line. Easier to read and makes line-based editing easier.

Feel free to disagree though! I find all of these add value individually, and would rather have either one of them than none at all.

Also, this obviously can't be merged unless all other PRs have already been merged, or it would create a huge mess. So Draft for now.

Copy link
Member

@davesnx davesnx left a comment

Choose a reason for hiding this comment

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

Great effort @glennsl

; Event {name= "onAnimationIterationCapture"; type_= Animation}
; Event {name= "onTransitionEnd"; type_= Transition}
; Event {name= "onTransitionEndCapture"; type_= Transition} ]
[ Event { name = "onCopy"; type_ = Clipboard }
Copy link
Member

Choose a reason for hiding this comment

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

That case is really weird. ocamlformat and the highlight don't like each other.

Copy link
Collaborator

@jchavarri jchavarri left a comment

Choose a reason for hiding this comment

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

I guess this would be nice to have to fix ci issues in main.

glennsl added 4 commits March 14, 2022 23:38
to avoid formatting inconsistencies when using different versions,
and checks failing on CI but not locally etc.
@glennsl glennsl force-pushed the style/ocamlformat-tweaks branch from 343cb61 to 98af6f2 Compare March 14, 2022 22:40
@glennsl
Copy link
Contributor Author

glennsl commented Mar 14, 2022

Rebased on main

@glennsl glennsl marked this pull request as ready for review March 14, 2022 22:41
@jchavarri
Copy link
Collaborator

Thanks!

@jchavarri jchavarri merged commit 4cae40f into ml-in-barcelona:main Mar 15, 2022
@glennsl glennsl deleted the style/ocamlformat-tweaks branch March 15, 2022 07:36
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