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

Comments: Add Support for single space trailing comments #219

Open
awalterschulze opened this issue Nov 4, 2020 · 6 comments
Open

Comments: Add Support for single space trailing comments #219

awalterschulze opened this issue Nov 4, 2020 · 6 comments
Labels
enhancement New feature or request help wanted Extra attention is needed

Comments

@awalterschulze
Copy link
Member

After analyses we have decided that support for trailing comments would be a good idea https://github.com/WhatsApp/erlfmt/blob/master/doc/FormattingDecisionComments.md

a, % trailing comment

This does not include aligning comments, only trailing comments with a single space between the code and the comment.

This does however require a significant amount of work to add this support.
There are no promises on when/if this will be delivered.
We do however believe this will be a big improvement to the project.

Concerns include that this will require a lot of decisions to be make about design of layouts.
For example, when an expression ends in a comment that is too long to fit on one line, the fits function will see this line as breaking and it will break that group. Do you keep the comment on the same line as the last group or do you have a break between it and the last group resulting in it being formatted below the expression. This is just one of the tough decisions that will need to be made on this implementation journey.

Reworking the algorithm to move the comment to the line above, only in the case that it breaks would require inventing a new operator for the fitting algorithm. If we do this, we could probably write a paper for ICFP.

@mheiber
Copy link
Contributor

mheiber commented Nov 17, 2020

%

@zuiderkwast
Copy link

In Emacs erlang-mode (the standard indentation within OTP), trailing comment are placed on column 48. See https://erlang.org/doc/man/erlang.el.html#indent

In Emacs, you can press Alt+; to create a trailing comment.

@awalterschulze
Copy link
Member Author

Yes, we did an analyses and found this is not as popular as we had hoped :(
https://github.com/WhatsApp/erlfmt/tree/master/doc/FormattingDecisionComments
To see the graphs download the html file and open it in a browser https://github.com/WhatsApp/erlfmt/blob/master/doc/FormattingDecisionComments/columns.html

@zuiderkwast
Copy link

Thanks for the reply! I agree, it's not very commonly seen. I just wasn't sure if you were aware of this at all.

@KennethL
Copy link

It would be interesting to format all of the OTP code with erlfmt and I am investigating when, how and if we should do that.
One of the obstacles is that end of the line comments are moved to a separate line by erlfmt.
We would like erlfmt to keep end of line comments as is (on the same line) at least when possible.
If the line get too long there are several alternatives:

  1. Do as today and put the comment on a separate line before the code
  2. emit a warning or error that the line is too long because of a comment (possibly with an option to erlfmt) and let the programmer chose where to have the comment or to shorten it

@awalterschulze
Copy link
Member Author

Ecstatic to hear that you are considering formatting OTP with erlfmt
We do believe supporting keeping end of line comments as is, would be an improvement to erlfmt. Finding more users that would want this helps to increase the priority, so thank you so much for commenting here :)

  1. Moving a comment to above a line, only when a break is required, is not possible without inventing a new operator and possibly publishing a paper. It is not trivial.
  2. emiting a warning of too long line, with an option to ignore, would be possible, I think. I think this would require modifying the fits function to ignore comment lengths or something like that and probably a lot more case specific work, but technically possible. But we do not want to create options to support different formats. The option would only turn the warnings on or off.

We believe this work is backwards compatible: would not result in already formatted files changing, that is why it is not included in milestone 1.0.0 yet.

@awalterschulze awalterschulze added the help wanted Extra attention is needed label Mar 31, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

4 participants