Add support for default values to TYPEDSIGNATURES
#170
Conversation
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.
|
Thanks @JamesWrigley! Would it make sense to also apply this to |
src/parsing.jl
Outdated
| # Parse an argument with a type annotation. | ||
| # Example input: `x::Int` | ||
| function parse_arg_with_type(arg_expr::Expr) | ||
| if arg_expr.head != :(::) |
There was a problem hiding this comment.
There's Meta.isexpr which would be preferable to use to keep things consistent instead of reaching into the .head field.
There was a problem hiding this comment.
Same applies to elsewhere that uses .head == comparisons.
There was a problem hiding this comment.
Good point, fixed in 1c80865.
Ah, yes you're right. I never use that so I forgot about it 😅 Also one question about the tests, the call to |
|
Applied to |
It won't be available when used in templates.
|
(I'll review when I'm back from vacation in a couple of weeks @JamesWrigley) |
| elseif n_expr_args == 2 | ||
| # 'x::Int' | ||
| ASTArg(; name=arg_expr.args[1], type=arg_expr.args[2]) | ||
| end |
There was a problem hiding this comment.
Adding an explicit failure branch here (even though we know that ::s always have one or two arguments) would be good.
| elseif obj isa Symbol || (obj isa Expr && isempty(obj.args)) | ||
| # Base case: this is the end of a branch in the expression tree | ||
| return nothing | ||
| end |
There was a problem hiding this comment.
There's no case that covers any other potential types found in ASTs. This just assumes that it is now an Expr. Are we sure that that is a safe assumption to make?
There was a problem hiding this comment.
Good point, probably not. Should be fixed in f1c9145.
|
I'm looking into the failure on 1.10 👀 |
|
Ok, should be fixed in 1efecf6. The problem is that |
|
The issue with Julia 1.0 is that |
Ideally not just because of a test-only dependency needing to have a more recent version. Was using |
|
It's by far the easiest, yeah. It's used for getting the |
|
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? 😅 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. |
What are the kinds of differences you're seeing here? Perhaps there are ways to write the tests to not encounter them? |
This is done by making
TypedMethodSignaturesinterpolatable 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 inTYPEDSIGNATURES, 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 😢), 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.