-
Notifications
You must be signed in to change notification settings - Fork 90
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
Preview ocamlformat 0.27.0 preview1 #1227
base: master
Are you sure you want to change the base?
Conversation
The aim of this preview is to gather feedback. Changelog can be found here: https://github.com/ocaml-ppx/ocamlformat/blob/main/CHANGES.md
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great to have a release of ocamlformat with support for 5.2!
Most of the changes are related to the parse-docstrings
being true by default. Making two commits (one setting parse-docstrings=false
and version=0.27.0
and the second one removing parse-docstrings=false
) would make it easier to see what part of the code change.
I left some very minor comments, it looks good to me.
{- module | ||
system | ||
documentation | ||
including |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is strange doc formatting!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed: ocaml-ppx/ocamlformat#2607
`Simple_reference of | ||
string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line break seems unnecessary!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed: ocaml-ppx/ocamlformat#2606
match kind with Doc _ -> false | _ -> true | ||
match kind with | ||
| Doc _ -> false | ||
| _ -> true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are match always on multiple lines now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is due to the comment being on its own line:
let _ =
(* foo *)
match kind with
| Doc _ -> false
| _ -> true
let _ = (* foo *) match kind with Doc _ -> false | _ -> true
I think it makes sense, if this match is important enough to require a comment, it should also be made more readable and editable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes a lot of sense. The comment was so big that I did not notice it.
Thanks for al the fixes!
analysis, and hence these types are capable of representing values that will | ||
be rejected by further stages, for example, invalid references or headings that | ||
are out of range. *) | ||
{{:https://ocaml.org/releases/4.12/htmlman/ocamldoc.html} The manual} for a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The space in {{!ref} content}
and {{:link} content}
seems a bit weird to me!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also fixed: ocaml-ppx/ocamlformat#2608
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the review! I'll split the commits for the final version, for now this is generated by a script.
match kind with Doc _ -> false | _ -> true | ||
match kind with | ||
| Doc _ -> false | ||
| _ -> true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is due to the comment being on its own line:
let _ =
(* foo *)
match kind with
| Doc _ -> false
| _ -> true
let _ = (* foo *) match kind with Doc _ -> false | _ -> true
I think it makes sense, if this match is important enough to require a comment, it should also be made more readable and editable.
`Simple_reference of | ||
string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed: ocaml-ppx/ocamlformat#2606
{- module | ||
system | ||
documentation | ||
including |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed: ocaml-ppx/ocamlformat#2607
The aim of this preview is to gather feedback.
Changelog can be found here: https://github.com/ocaml-ppx/ocamlformat/blob/main/CHANGES.md