DocStringExtensions.jl icon indicating copy to clipboard operation
DocStringExtensions.jl copied to clipboard

Add support for default values to `TYPEDSIGNATURES`

Open JamesWrigley opened this issue 1 year ago • 14 comments

This is done by making TypedMethodSignatures interpolatable and giving it the Expr of the thing the docstring is bound to. From the AST we can parse each arguments name, type annotation, default value, and whether or not it's variadic. Currently only the default value is used in TYPEDSIGNATURES, though in the future it might be an idea to use the type information as well.

I think this could do with some more tests (parsing is hard :cry:), but it's pretty close to being complete. Fixes #107, fixes #19. Would make #97 possible if we're ok with getting the types from the AST.

JamesWrigley avatar Aug 15 '24 18:08 JamesWrigley

Thanks @JamesWrigley! Would it make sense to also apply this to SIGNATURES rather than just TYPEDSIGNATURES?

MichaelHatherly avatar Aug 16 '24 08:08 MichaelHatherly

Thanks @JamesWrigley! Would it make sense to also apply this to SIGNATURES rather than just TYPEDSIGNATURES?

Ah, yes you're right. I never use that so I forgot about it :sweat_smile: Also one question about the tests, the call to DSE.parsedocs(DSE) at the very end is failing because TypedMethodSignatures.expr is nothing. I guess that means that the interpolation didn't execute? How could I fix that? I've tried it with other docstrings and for those the interpolation definitely works.

JamesWrigley avatar Aug 16 '24 09:08 JamesWrigley

Applied to SIGNATURES in de0f36b. And I think I know why the templating tests are failing, I observed that within a template template_hook(::LineNumberNode, ::Module, ::Symbol) was being called, which will fall back to the default Core.atdoc(). I think the problem is that the Expr and the docstring are defined in separate places so template_hook(::LineNumberNode, ::Module, docstr, ::Expr) will never be called and we'll never capture the expression. Is there a way around that? Or will we just have to live with never getting Expr's in templates?

JamesWrigley avatar Aug 18 '24 14:08 JamesWrigley

Made the expr optional in fe01fe3, and made the parser more robust in 1532071. I think this is ready to merge now :crossed_fingers:

(BTW if you approve I can rebase before merging to clean up the commit history)

JamesWrigley avatar Aug 19 '24 14:08 JamesWrigley

(I'll review when I'm back from vacation in a couple of weeks @JamesWrigley)

MichaelHatherly avatar Aug 24 '24 09:08 MichaelHatherly

I'm looking into the failure on 1.10 :eyes:

JamesWrigley avatar Sep 12 '24 12:09 JamesWrigley

Ok, should be fixed in 1efecf6. The problem is that Base.return_types() was failing on 1.10 for a generated function, which it is documented to be able to do. I figured that a try-catch is the safest way to fix it in case people do something like use a macro that creates a @generated expr internally.

JamesWrigley avatar Sep 12 '24 14:09 JamesWrigley

The issue with Julia 1.0 is that CodeTracking can't be installed for the tests. Do we dare just bump the minimum to Julia 1.10 since that's going to be the new LTS?

JamesWrigley avatar Sep 16 '24 09:09 JamesWrigley

Do we dare just bump the minimum to Julia 1.10 since that's going to be the new LTS?

Ideally not just because of a test-only dependency needing to have a more recent version. Was using CodeTracking the only way you've found to be able to write those tests?

MichaelHatherly avatar Sep 16 '24 09:09 MichaelHatherly

It's by far the easiest, yeah. It's used for getting the Expr's for methods. Otherwise I think we'd need to either vendor in the relevant parts from CodeTracking.jl or rewrite the tests to pass Expr's to them explicitly. I tried lowering the compat bounds to 1.0 in 79783e4, let's see if that works :crossed_fingers:

JamesWrigley avatar Sep 16 '24 10:09 JamesWrigley

Looks to have gotten further, there's a syntax issue with some namedtupled code now, probably just needs rewriting to a form that supports 1.0.

MichaelHatherly avatar Sep 16 '24 10:09 MichaelHatherly

How about bumping to Julia 1.6? :sweat_smile: Supporting Julia 1.0 is a bit tricky because this line gives different results from later versions: https://github.com/JamesWrigley/DocStringExtensions.jl/blob/79783e4e0228fcd82a427a743d4c7c9d906b2d7d/src/utilities.jl#L360-L362

I'm guessing we weren't hitting that codepath before. FWIW ebb95a9 has the changes that pass the tests on 1.6.

JamesWrigley avatar Sep 17 '24 10:09 JamesWrigley

Supporting Julia 1.0 is a bit tricky because this line gives different results from later versions:

What are the kinds of differences you're seeing here? Perhaps there are ways to write the tests to not encounter them?

MichaelHatherly avatar Sep 17 '24 10:09 MichaelHatherly

Posting here to add my support for this! Hopefully this PR can be revived :heart:

tomchor avatar Apr 09 '25 23:04 tomchor

I am also posting my support for this PR! I was thinking exactly the same as #19 and #107.

navidcy avatar Nov 13 '25 20:11 navidcy