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

Question: What does wrap-comments do? #2642

Closed
mbarbin opened this issue Jan 6, 2025 · 2 comments · Fixed by #2645
Closed

Question: What does wrap-comments do? #2642

mbarbin opened this issue Jan 6, 2025 · 2 comments · Fixed by #2645

Comments

@mbarbin
Copy link
Contributor

mbarbin commented Jan 6, 2025

This is a question about the formatting option wrap-comments=true.

I think I am misunderstanding its intent. I thought this would be an option to force inserting newline characters in regular non-docstring comments, so they fit into the margin. However, experimenting with this, this doesn't seem to be the case. I am not sure whether I should file a bug report, or I misunderstood how the option can be used.

Here is my config:

version = 0.27.0
profile = janestreet
wrap-comments = true

Here was an attempt at writing a long comment, with the hope that it would be automatically wrapped on fmt:

(* Hello, this file has [wrap-comments = true] enabled via its [.ocamlformat] config, but I am not sure what changes does it make. This comment for example is not wrapped when this file is formatted. Sounds like the setting is ignored. Or how long should a comment for it to be wrapped? I am keeping this comment here for the time being to monitor whether there'd be changes on this front. *)
@Julow
Copy link
Collaborator

Julow commented Jan 7, 2025

This changed in #2371. Regular comments are formatted as doc-comments with the janestreet profile, independently of the wrap-comments option. But then, because parse-docstrings is not enabled, the comments are not formatted. There seems to be a logical problem here.
An other thing is that doc-comments don't wrap with the janestreet profile, and that cannot be changed with an option.
I may have fixed that in #2645

@mbarbin
Copy link
Contributor Author

mbarbin commented Jan 9, 2025

Thank you for looking into the issue!

Asterisk

I built a local version of ocamlformat including this fix:

$ ocamlformat --version
0.27.0-1-g4c94d48

and then tried it out with this config:

version=0.27.0-1-g4c94d48
ocaml-version=5.2
profile=janestreet
parse-docstrings=true
wrap-comments=true

I confirm now that a file like the following is stable through fmt. Thank you!

(* Hello.
 * This is a comment with asterisk chars
 * at the beginning of every lines.
 *
 * Yay!
 *)
let () = ()

Incidentally as I was editing the file manually, I came across this case:

(* Hello.
   * This is a comment with asterisk chars
 * at the beginning of every lines.
 *
 * Yay!
 *)
 let () = ()
$ ocamlformat mini_fmt.ml
ocamlformat: Cannot process "mini_fmt.ml".
  Please report this bug at https://github.com/ocaml-ppx/ocamlformat/issues.
  BUG: comment changed.
File "mini_fmt.ml", lines 1-6, characters 0-3:
Error: comment (*  Hello.
   * This is a comment with asterisk chars
 * at the beginning of every lines.
 *
 * Yay!
  *) dropped.
  BUG: comment changed.
File "mini_fmt.ml", lines 1-6, characters 0-2:
Error: comment (*  Hello.
   * This is a comment with asterisk chars
   * at the beginning of every lines.
   *
   * Yay!
 *) added.

As in, if the asterisk are not originally aligns, something unexpected happens. Reporting for your consideration. cours Asterixme, cours!!

Wrap-comments

I then tried to copy my long one line comment in another file, both as regular or doc-strings comment. I does not wrap.

An other thing is that doc-comments don't wrap with the janestreet profile, and that cannot be changed with an option.

I am not sure if the current fix intended to fix this as well. If it did, something is not working as expected.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants