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: Rethink formatting of function with spec and comment #94

Closed
awalterschulze opened this issue Aug 13, 2020 · 8 comments
Closed
Labels
duplicate This issue or pull request already exists

Comments

@awalterschulze
Copy link
Contributor

Given the following input

-spec get_service_name(client:api()) -> atom().
get_service_name(_API) -> name. % sync with name.app.src

It is formatted too

-spec get_service_name(client:api()) -> atom().
% sync with name.app.src
get_service_name(_API) -> name.

Better options are:

-spec get_service_name(client:api()) -> atom().
get_service_name(_API) -> name. % sync with name.app.src
-spec get_service_name(client:api()) -> atom().
get_service_name(_API) ->
  % sync with name.app.src
  name.
% sync with name.app.src
-spec get_service_name(client:api()) -> atom().
get_service_name(_API) -> name.

Where number 1 has gotten a vote and number 3 has gotten at least one vote and one settle.

@michalmuskala
Copy link
Member

michalmuskala commented Aug 13, 2020

The main complexity in this seems to be - what to do in cases where the comment is in the middle in the first place, e.g.

-spec get_service_name(client:api()) -> atom().
% sync with name.app.src
get_service_name(_API) -> name.

and also what to do in case of multiple clauses

-spec get_service_name(client:api()) -> atom().
get_service_name(#{override: Name}) -> Name; % override
get_service_name(_) -> name. % sync with name.app.src

For the first one, I don't think we should be changing it, for the second one, I'm really not sure what would be the right approach.

@michalmuskala michalmuskala added the bug Something isn't working label Aug 13, 2020
@ruippeixotog
Copy link

The second case as an interesting one! It seems like a good argument to choose option 2 as a more general approach (since comments may be related to specific clauses).

@TheGeorge
Copy link

I think comment placement should not be changed significantly by the formatter. The coder usually has something in mind placing them. I think it's wrong moving them to other places.

I think 1. is the only right option.

@awalterschulze awalterschulze changed the title Rethink formatting of function with spec and comment Comments: Rethink formatting of function with spec and comment Sep 22, 2020
@awalterschulze
Copy link
Contributor Author

We have had another report about comments with spec
"""
I usually write %% @private between a function's -spec(_). and its declaration, like so:

-spec add(A :: integer(), B :: integer()) -> integer().
%% @private
add(A, B) ->

erlfmt prefers it like

-spec add(A :: integer(), B :: integer()) -> integer().
    %% @private
add(A, B) ->

"""

@michalmuskala
Copy link
Member

michalmuskala commented Sep 23, 2020

This last case looks like a bug to me and separate from the initial issue raised here

@awalterschulze
Copy link
Contributor Author

I agree :)

@awalterschulze
Copy link
Contributor Author

I have retested with master version of erlfmt and the bug does not occur any more.

@awalterschulze
Copy link
Contributor Author

After doing more analyses on comments, we have decided that adding support for trailing comments is a good idea #219

Doing it will require a lot of work, so no promises on when this will be delivered, but I believe we can consider this issue a duplicate, because if we support trailing comments, then this wouldn't have been an issue.

I hope that makes sense.

@awalterschulze awalterschulze added duplicate This issue or pull request already exists and removed bug Something isn't working labels Nov 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
duplicate This issue or pull request already exists
Projects
None yet
Development

No branches or pull requests

4 participants