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

@format collides with edoc #243

Open
elbrujohalcon opened this issue Apr 20, 2021 · 26 comments
Open

@format collides with edoc #243

elbrujohalcon opened this issue Apr 20, 2021 · 26 comments
Labels
bug Something isn't working
Milestone

Comments

@elbrujohalcon
Copy link
Collaborator

As detected by @awalterschulze on WhatsApp/erlfmt#277, we probably need a new tag format.

@elbrujohalcon elbrujohalcon added the bug Something isn't working label Apr 20, 2021
@elbrujohalcon elbrujohalcon added this to the Eventually 🙄 milestone Apr 20, 2021
@awalterschulze
Copy link
Collaborator

Apparently it is just a warning, but still could be worth cleaning up.

@elbrujohalcon
Copy link
Collaborator Author

elbrujohalcon commented Apr 20, 2021

Indeed. It looks like this…

/path/to/file.erl, function my_fun/0: at line 3: warning: tag @format not recognized.

…or…

/path/to/file.erl,  in module header: at line 3: warning: tag @format not recognized.

…even if you write it like this:

%%% @doc This module description.
%%%      More docs.
%%% @end
%%% @format ignore.

@elbrujohalcon
Copy link
Collaborator Author

But that means that we can't just use another word. We need a different way to tag stuff… and we risk edoc picking that up and adding it to the documentation. Not good.

@awalterschulze
Copy link
Collaborator

That is a very good point, hmmm.

@awalterschulze
Copy link
Collaborator

I was thinking something like #format , but then as you say we risk edoc putting this into the generated documentation.

@elbrujohalcon
Copy link
Collaborator Author

We could use user-defined macros, i.e. {@format …} but that would require users who want to skip the warning to add this to their rebar.config files:

{edoc_opts, [{def, [{format, ""}]}]}.

Not sure if a plugin can auto-add that thing so the user doesn't need to do it manually.

@awalterschulze
Copy link
Collaborator

Aha okay, but if they do not add it, they still just get a warning right?

@elbrujohalcon
Copy link
Collaborator Author

Right!

@awalterschulze
Copy link
Collaborator

Then this sounds very promising.

@awalterschulze
Copy link
Collaborator

I have not checked this, but apparently edoc does allow comments inside edoc, such that the following won't be picked up by edoc as documentation, since it is a comment inside a comment.

% % @format

Notice the space between the two percentages.

@paulo-ferraz-oliveira
Copy link
Contributor

Haha, so hackish. I love it. I wonder if documenting it as such, though, will be more of a headache than not. (even with a "Caveats" section, developers don't often read doc.s).

@awalterschulze
Copy link
Collaborator

Yeah it is true and probably the main concern, but whatever we pick this would be the new default for --insert-pragma and --require-pragma would check for both.

@awalterschulze
Copy link
Collaborator

I will do a bit of checking of these options on Wednesday, I hope.

@elbrujohalcon elbrujohalcon modified the milestones: Eventually 🙄, 1.1.0 Apr 28, 2021
@awalterschulze
Copy link
Collaborator

If I add the comment

%% {@format}

and run

$ rebar3 shell --start-clean
> edoc:files(["/Users/walterschulze/code/src/github.com/WhatsApp/erlfmt/src/erlfmt.erl"]).

Then I don't get a warning and I haven't even added any edoc_opts to my rebar.config
Are you sure the {edoc_opts, [{def, [{format, ""}]}]}. is needed?

@awalterschulze
Copy link
Collaborator

Ah I see, I only get the warning if it is after a %% @doc comment.

@elbrujohalcon
Copy link
Collaborator Author

For context, I tried with rebar3 edoc not within a shell.

@awalterschulze
Copy link
Collaborator

Ah nice, yes even with rebar3 edoc I only get the warning for

%% @doc
%% {@format}
%% @end

And the warning goes away if I add

{edoc_opts, [{def, [{format, ""}]}]}.

to the rebar.config

If however I write only

%% {@format}

without any @doc before it and without a rebar.config addition, then I do not get a warning.

If I write

%% {@format}
%% @doc
%% bla
%% @end

without any @doc before it and without a rebar.config addition, then I also do not get a warning.

@elbrujohalcon
Copy link
Collaborator Author

elbrujohalcon commented Apr 28, 2021

What if you use @format (i.e. without the curly braces) in that same context, then?

@awalterschulze
Copy link
Collaborator

All configurations with %% @format with curly braces give a warning

in module header: at line 2: warning: tag @format not recognized.

@awalterschulze
Copy link
Collaborator

So looks like both options:

%% {@format}

and

% % @format

would not create any warnings from edoc or extra text in generated by edoc

@awalterschulze
Copy link
Collaborator

awalterschulze commented Apr 28, 2021

From a require_pragma standpoint is easy to support all variants.
It is only a question of which one would be recommended and inserted with insert pragma

@elbrujohalcon
Copy link
Collaborator Author

Yeah… on this project we don't care about pragmas… and we already have support for -format as an attribute.
I think for us the easiest one will be % % @format and it will be more than enough.

@awalterschulze
Copy link
Collaborator

I am going to take a stab at implement the single space and see how it goes.

@awalterschulze
Copy link
Collaborator

awalterschulze commented Apr 28, 2021

Actually forgot that @format does not have to be in the first comment, so it actually does matter, since {@format} can be after @doc, so %% % @format is the better choice.

@awalterschulze
Copy link
Collaborator

WhatsApp/erlfmt#281

@awalterschulze
Copy link
Collaborator

Decided on %%% % @format for a default of mimicking the number of percentages if @format follows some other comments, for example

%% bla
%% %@format

This was after we did a new analyses, to find that triple percentage is actually quite popular, before the module pragma
https://github.com/WhatsApp/erlfmt/blob/master/doc/FormattingDecisionComments/module_comments.md

@elbrujohalcon elbrujohalcon modified the milestones: 1.1.0, 1.1.1 Apr 11, 2022
@elbrujohalcon elbrujohalcon modified the milestones: 1.1.1, 1.1.2 May 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants