-
Notifications
You must be signed in to change notification settings - Fork 23
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
Hex package preparations #147
Conversation
Erlang does not accept funs as literals to be parsed by a syntax tree, so documentation for required_variable won’t pass if it’s using fun objects. https://github.com/erlang/otp/blob/f820bad7bed34cc4365bb9cac56eaa84c9a5bddc/lib/syntax_tools/src/erl_syntax.erl#L7537
@@ -16,9 +16,9 @@ | |||
-required_variable(#{name => var1, description => "var1"}). | |||
-required_variable(#{name => var2, description => "var2", default_value => def2}). | |||
-override_variable(#{name => var2, description => "var2", default_value => val2, | |||
verification => [def2, val2], update => fun ?MODULE:update_fn/2}). | |||
verification => [def2, val2], update => {?MODULE, update_fn, 2}}). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we have to test both cases here, MFA tuple and full function reference.
@@ -20,15 +20,15 @@ | |||
#{name => var3, description => "var3", default_value => def3, | |||
verification => [def3, another_atom]}, | |||
#{name => var4, description => "var4", default_value => def4, | |||
verification => fun ?MODULE:verify_value/1} | |||
verification => {?MODULE, verify_value, 1}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we have to test both cases here, MFA tuple and full function reference.
@@ -117,11 +116,13 @@ handle_continue(maybe_run_fn, State) -> | |||
NewState = maybe_run_fn(State), | |||
{noreply, NewState, timeout(NewState)}. | |||
|
|||
-spec format_status(term(), term()) -> term(). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it anyhow better than automatically generated spec?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Types aren't specified and ex_doc was annoying me with warnings of an exported function without specs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is it possible to exclude that function from ex_doc, and the whole file as well? it is an internal module.
#{name => var2, description => "description2", default_value => def2, | ||
verification => fun ?MODULE:test_verification_function/1}, | ||
verification => {?MODULE, test_verification_function, 1}}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we have to test both cases here, MFA tuple and full function reference.
%% 'fun module:function/arity' format. Note that it must be an exported function. | ||
%% usage of 'fun function/arity' format in the module attribute simply will | ||
%% not pass compilation. | ||
%% verification function must be supplied as an mfa of the format |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment is incorrect, because there are 2 formats supported now.
@@ -1,6 +1,143 @@ | |||
%%============================================================================== | |||
%% Copyright 2023 Erlang Solutions Ltd. | |||
%% Licensed under the Apache License, Version 2.0 (see LICENSE file) | |||
%% @doc This module allows to synchronize the users and act on groups of them. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Being honest, I'm not a big fun of such big documentation blocks in the header. I would rather keep a separated markdown doc (just maybe move it into the guides
directory, so all the documentation is at one place).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have an idea then.
@@ -1,6 +1,148 @@ | |||
%%============================================================================== | |||
%% Copyright 2023 Erlang Solutions Ltd. | |||
%% Licensed under the Apache License, Version 2.0 (see LICENSE file) | |||
%% @doc This module allows to synchronize the users and act on groups of them. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Being honest, I'm not a big fun of such big documentation blocks in the header. I would rather keep a separated markdown doc (just maybe move it into the guides
directory, so all the documentation is at one place).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for addressing all the comments.
Throttle and coordinator documentation was moved to their respective modules, which reduces the redundancy of the documentation as specs for functions that also have specs.
Because ex_doc will clear and output its produce to the
doc/
dir, files from such dir have been moved toguides/
.The most controversial change could be the last commit, see commit message for reasonale.
To build locally you can fetch the branch and run
And see in your browser
amoc/doc/index.html
.How it looks locally: