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

comment the @format pragma to avoid edoc warnings #281

Merged
merged 8 commits into from
May 5, 2021
Merged

Conversation

awalterschulze
Copy link
Contributor

@awalterschulze awalterschulze commented Apr 28, 2021

Fixes #277

Some context: Marc-Andre has shown me an analyses of triple comments that removes the bias in my analyses, that shows that they are actually popular in erlang before the module attribute. We will work on pushing this analyses to the repo, but I think we should adopt this style in insert pragma

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Apr 28, 2021
src/erlfmt.erl Outdated
Comment on lines 147 to 151
[{comment, #{location => Loc, end_location => Loc}, ["%%% % @format", ""]}];
_ ->
{comment, Loc, LastComments} = lists:last(PreComments),
lists:droplast(PreComments) ++
[{comment, Loc, LastComments ++ ["%% @format"]}]
[{comment, Loc, LastComments ++ ["%%% % @format"]}]
Copy link
Member

@michalmuskala michalmuskala May 4, 2021

Choose a reason for hiding this comment

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

I feel like the proper way to do this would be to detect whether to use %% or %%%, In particular something like:

{Prefix, _} = string:take(lists:last(LastComments), "%"),
[{comment, Loc, LastComments ++ [Prefix ++ " % @format"]}]

I don't have a preference for what should be the default in case there weren't any comments already.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could, but we still need a default for the case where there are no comments yet and in this case I think we should do 3 comments, given this new analyses: #283

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have added your suggestion :)

src/erlfmt.erl Outdated
Comment on lines 206 to 211
replace_pragma_comment_block([]) ->
[];
replace_pragma_comment_block(["%% @format" | Tail]) ->
["%%% % @format" | Tail];
replace_pragma_comment_block([Head | Tail]) ->
[Head | replace_pragma_comment_block(Tail)].
Copy link
Member

@michalmuskala michalmuskala May 4, 2021

Choose a reason for hiding this comment

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

I'm not convinced about forcing the triple comment. What if the surrounding comment uses only double? It would be strange to end up with something like:

%% Some licence info
%%
%%% % @format
%%
%% @doc ....

e.g. where this would happen: https://github.com/erlang/otp/blob/master/lib/edoc/src/edoc.erl

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes we could replace it with the number of percentages that was there.

Copy link
Contributor Author

@awalterschulze awalterschulze May 4, 2021

Choose a reason for hiding this comment

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

I have also addressed this with a default of 3, but if some other number is detected we prefer that.

src/erlfmt.erl Outdated
Comment on lines 203 to 204
[] ->
replace_pragma_comment_block(Prefix, Rest);
Copy link
Member

Choose a reason for hiding this comment

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

Can this happen? I don't think we ever produce empty comment blocks and replace_pragma_comment_block/2 never drops any list elements

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch, I updated the code.

@awalterschulze awalterschulze merged commit 6a0daf4 into master May 5, 2021
@awalterschulze awalterschulze deleted the new_pragma branch May 5, 2021 16:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add new pragma alternative
3 participants