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

Add docstring support to @agent #715

Closed
veddox opened this issue Dec 9, 2022 · 8 comments · Fixed by #717
Closed

Add docstring support to @agent #715

veddox opened this issue Dec 9, 2022 · 8 comments · Fixed by #717
Labels

Comments

@veddox
Copy link
Contributor

veddox commented Dec 9, 2022

Attaching a docstring to an @agent definition fails with an error: `'@agent' not documentable. See 'Base.@doc' docs for details.' This is unexpected and problematic, as documenting agent types is important.

The @agent documentation states: "You can use the @doc macro from Julia to document the generated struct if you wish so." However, this statement (1) is easy to overlook, and (2) does not provide an example. (I did not get @doc to work in conjunction with @agent, and could not find a working example.)

My understanding is that docstring support can be added to a macro using @__doc__. Adding this to @agents would allow modellers to better document their code.

@Datseris
Copy link
Member

Datseris commented Dec 9, 2022

To add a docstring you'd do:

@agent MyAgent .....
....
end

@doc "lalalala"
MyAgent

But yes, it would be nice to have a clearer example in the docstring, please consider a PR!

I don't know how we could modify @agent to allow the traiditional docstirng syntax though.

@veddox
Copy link
Contributor Author

veddox commented Dec 16, 2022

I've now used Core.@__doc__ in one of my own macros, and it seems quite straightforward. You simply insert it in front of the expression that is to be documented. For @agent, that should be the struct, as follows:

macro agent(new_name, base_type, extra_fields)
    quote
        let
            # Here we collect the field names and types from the base type
            # Because the base type already exists, we escape the symbols to obtain it
            base_fieldnames = fieldnames($(esc(base_type)))
            base_fieldtypes = [t for t in getproperty($(esc(base_type)), :types)]
            base_fields = [:($f::$T) for (f, T) in zip(base_fieldnames, base_fieldtypes)]
            # Then, we prime the additional name and fields into QuoteNodes
            # We have to do this to be able to interpolate them into an inner quote.
            name = $(QuoteNode(new_name))
            additional_fields = $(QuoteNode(extra_fields.args))
            # Now we start an inner quote. This is because our macro needs to call `eval`
            # However, this should never happen inside the main body of a macro
            # There are several reasons for that, see the cited discussion at the top
            expr = quote
                Core.@__doc__ mutable struct $name <: AbstractAgent # <<< ADD @__doc__ HERE <<<
                    $(base_fields...)
                    $(additional_fields...)
                end
            end
            # @show expr # uncomment this to see that the final expression looks as desired
            # It is important to evaluate the macro in the module that it was called at
            Core.eval($(__module__), expr)
        end
    end
end

I haven't managed to get a development version of Agents.jl working yet to test this, but it should do it.

@Datseris
Copy link
Member

I haven't managed to get a development version of Agents.jl working yet to test this, but it should do it.

What do you mean? Why not? It's great that you found a potential solution but I am out of bandwidth at the moment so a PR is much much appreciated!

@veddox
Copy link
Contributor Author

veddox commented Dec 19, 2022

What do you mean? Why not?

Sorry about that - I still don't fully understand Julia's package system and hadn't yet figured out how to get a local version of a package to run. But I've got everything set up now 👍

a PR is much much appreciated!

I'm working on it. Unfortunately, I discovered that the solution suggested above doesn't work due to the nested quoting. @__doc__ has to be prepended to the expression that is to have the docstring attached to it, but this only works on the macro's top level. So I can do @__doc__ expr = quote, but not @__doc__ mutable struct $name <: AbstractAgent. However, doing the former then attaches the docstring to the local variable expr (or rather, it's gensymed equivalent), not to the newly created struct, where it's needed.

I'll see if I can figure out how to by-pass this issue. Otherwise, I'll just add a documentation note to @agent about how to document it with @doc, like you suggested.

@Datseris
Copy link
Member

Feel free to open a question at Discourse!

@veddox
Copy link
Contributor Author

veddox commented Dec 19, 2022

I commented on this recent question: https://discourse.julialang.org/t/access-docstring-within-a-macro/90362

Let's see if anyone has any bright ideas.

@MichaelHatherly
Copy link

You'll need to do something like the following because of the use of eval and quoted quotes within the macro body. Docs.namify is used since it appears new_name can be a :curly expression and we just want the bare Symbol to document. I've also added a nothing at the end of the body so that you get the same returned value that eval on a struct expression gives, otherwise having a documented expression as the last expression ends up returning the documented Docs.Binding value instead, which you probably don't want.

diff --git a/src/core/agents.jl b/src/core/agents.jl
index e72e978..7771e13 100644
--- a/src/core/agents.jl
+++ b/src/core/agents.jl
@@ -197,6 +197,8 @@ macro agent(new_name, base_type, extra_fields)
             # It is important to evaluate the macro in the module that it was called at
             Core.eval($(__module__), expr)
         end
+        Core.@__doc__($(esc(Docs.namify(new_name))))
+        nothing
     end
 end
 # TODO: I do not know how to merge these two macros to remove code duplication.
@@ -234,6 +236,8 @@ macro agent(new_name, base_type, super_type, extra_fields)
             # It is important to evaluate the macro in the module that it was called at
             Core.eval($(__module__), expr)
         end
+        Core.@__doc__($(esc(Docs.namify(new_name))))
+        nothing
     end
 end

I've not thoroughly tested the above, so there might be some additional changes you'd need to do to make it work in every case that @agent needs to handle. The test suite does pass though, so doesn't appear to have broken anything at least.

If you want to be able to do documentation of fields of the resulting struct then the entire macro will need to be rewritten as far as I can tell to avoid the eval, which from what I can tell probably wouldn't be all that simple to do unfortunately.

veddox added a commit to veddox/Agents.jl that referenced this issue Dec 19, 2022
Fixes issue JuliaDynamics#715. With thanks to Michael Hatherly!
@veddox
Copy link
Contributor Author

veddox commented Dec 19, 2022

Oh wonderful, thank you very much! I spent most of the day trying to fix this, didn't think it would turn out to be so easy... I hadn't thought of simply @__doc__ing the name symbol 🤦‍♂️ (Though even if I had, I'd probably have missed the escaping and namifying.) Well, guess I learnt something again 🙂

Datseris pushed a commit that referenced this issue Dec 21, 2022
* Added docstring support to @agent

Fixes issue #715. With thanks to Michael Hatherly!

* Removed separate `@doc` calls with `@agent`

`@agent` can now take a docstring directly, instead of relying on an
additional call to `@doc`.

* Removed superflous `Core.@__doc__`

* Removed superfluous comment

* Added test for docstring capture

* Bumped patch number
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants