DocStringExtensions.jl
DocStringExtensions.jl copied to clipboard
Add support for default values to `TYPEDSIGNATURES`
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.
Thanks @JamesWrigley! Would it make sense to also apply this to SIGNATURES rather than just TYPEDSIGNATURES?
Thanks @JamesWrigley! Would it make sense to also apply this to
SIGNATURESrather than justTYPEDSIGNATURES?
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.
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?
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)
(I'll review when I'm back from vacation in a couple of weeks @JamesWrigley)
I'm looking into the failure on 1.10 :eyes:
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.
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?
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?
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:
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.
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.
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?
Posting here to add my support for this! Hopefully this PR can be revived :heart:
I am also posting my support for this PR! I was thinking exactly the same as #19 and #107.