Support function return types in longdef/shortdef #35

cstjean · 2017-03-30T19:08:38Z

I also have a function that uses MacroTools to parse any function definition and return (name, arg, kwargs, body, return_type). Would that be accepted?

cstjean · 2017-03-31T01:33:18Z

The way I use these functions, I just want to transform an expression, not code-walk over the entire tree. Would you mind if I factored out the @match code into a separate function?

MikeInnes · 2017-04-17T11:19:08Z

Looks good, thanks! It would probably make sense to have longdef1(ex) = ...; longdef = prewalk(longdef1). For the test issue, would flatten/striplines help? I think the test is fine as is but if you want to use the full version it should be possible. I think the general function parser would be a useful addition too.

cstjean · 2017-04-19T13:50:44Z

For the test issue, would flatten/striplines help?

I tried - still problematic. Let's leave it as is.

cstjean · 2017-04-26T17:00:36Z

Added parsedef and parsearg. I feel like parse isn't strictly the right verb to use, but I like it a bit more than destruc or match. Any preference? Returning a Nullable as the third value of parsearg is also not super-pleasing. Otherwise, this is ready to merge whenever you're ready to drop 0.4.

MikeInnes · 2017-05-10T17:30:02Z

src/utils.jl

+            push!(positional_args, x)
+        end
+    end
+    return (positional_args, kwargs)


I think this can be simplified a bit; :parameters will only be in the first slot, if at all. This should probably also handle expressions of the form f(a, b = c) correctly.

MikeInnes · 2017-05-10T17:30:16Z

src/utils.jl

+    return (positional_args, kwargs)
+end
+
+"""    parsedef(fdef)


How about splitdef?

MikeInnes · 2017-05-10T17:32:14Z

src/utils.jl

+end
+```
+
+and returns `(fname::Symbol, args::Vector{Any}, kwargs::Vector{Any}, body_block::Expr, return_type)`. `return_type` is `:Any` if not specified. """


This tuple is a bit long, I suggest a dict instead. It makes sense for certain keys to not be present if they're not written.

MikeInnes · 2017-05-10T17:33:13Z

src/utils.jl

+function parsedef(fdef)
+    @match longdef1(fdef) begin
+        (function fname_(args__) body_ end =>
+         (fname, split_kwargs(args)..., body, :Any))


I think it would make sense to use nothing here to differentiate from Any being explicitly written (or just leave out the key).

MikeInnes · 2017-05-10T17:34:48Z

src/utils.jl

+        default = Nullable(arg_expr.args[2])
+        arg_expr = arg_expr.args[1]
+    else
+        default = Nullable()


Using nothing would amount to the same here

But then we couldn't recognize f(x=nothing) from f(x), no? Maybe we could return a special NoVal() object instead? Nullables are cumbersome.

In f(x=nothing), :nothing would be a symbol, so that wouldn't be an issue. You could technically write f(x=$nothing) and break it though, so maybe nullable is the right route.

We can error in the f(x=$nothing) case. It seems fair enough for a marginal situation.

That seems reasonable enough to me.

MikeInnes · 2017-05-10T17:36:45Z

src/utils.jl

    end
-  end
+    if !(@capture(arg_expr, name_::typ_))


I think you could simplify this as something like @capture(ex, x_::T_ = default_ | x_::T_ | x_)

Cool, but then how can I distinguish no default from default being $nothing (and trigger the error I proposed above)?

MikeInnes · 2017-05-10T17:38:11Z

Yeah this is looking like a really useful set of functionality! Sorry for the late review but hopefully you can take a look at those couple of things and this should be good to go.

cstjean · 2017-05-10T18:25:26Z

Thank you for the comments, I'll do the necessary changes. My main use case for this functionality is to augment a function definition somehow, eg.

fname, args, kwargs, body_block, return_type = parsedef(fdef)
quote
    function $fname($(args...); $(kwargs...))::$return_type
         $body_block
    end
end

I think it would make sense to use nothing here to differentiate from Any being explicitly written (or just leave out the key).

It would be annoying to have to write $fname($(args...); $(kwargs...))::$(return_type==nothing ? :Any : return_type), and there's few cases where I'd want to distinguish between ::Any and no return type.

How about returning an object? It'll be shorter to type than a Dict.

immutable FunctionDefinition
    name
    args
    kwargs
    body_block
    return_type
end

MikeInnes · 2017-05-15T14:31:25Z

You could return Any as opposed to :Any to differentiate, though that strictly has the same issue as nothing above. A bonus of the dict approach is that you can write :(f()::$(get(def, :return, Any)) which is not too cumbersome.

cstjean · 2017-05-22T15:02:29Z

Updated. Thank you for all the suggestions, it looks better now.

cstjean · 2017-06-24T14:36:34Z

Any chance that we could revisit this? I've got three packages that rely on half-broken versions of this code...

I added support for where syntax, and it turned out rather messy. I can stash it away in a branch until Julia 0.7 if you prefer.

cstjean · 2017-08-09T20:13:14Z

I need a permanent home for this code, so unless I get some news from you, I'll create a new package in the next few days.

MikeInnes · 2017-08-11T12:22:22Z

Hey @cstjean, apologies for letting this slip through the cracks -- this is a great PR. I've merged it in 51391fb. Thanks a lot for the effort and patience!

cstjean · 2017-08-11T12:33:16Z

🎉 Thanks! My nemesis package, ExprTools.jl was ready to go and passed CI, but thankfully not registered yet.

I've made some improvements that aren't part of this PR. I'll make a new one today-ish, then we can tag?

MikeInnes · 2017-08-11T12:47:23Z

👍

MikeInnes reviewed May 10, 2017

View reviewed changes

src/utils.jl Outdated

return (positional_args, kwargs)

end

""" parsedef(fdef)

Copy link

Member

MikeInnes May 10, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about splitdef?

MikeInnes reviewed May 10, 2017

View reviewed changes

cstjean added 12 commits June 21, 2017 12:36

Support function return types in longdef/shortdef

a05095f

Factor out longdef1/shortdef1

ea6856a

Bump up REQUIRE to Julia 0.5

10f8b71

Add parsedef and parsearg

753fcfc

Simplify split_kwargs

a364ec2

Rename splitdef -> parsedef

b37cefc

Return a dict from splitdef

cf49ee9

Return nothing form parsearg when the default is missing

32f270a

Rename parsearg to splitarg

916ba5a

Simplify splitarg

23b4e06

Fix splitarg(:(x::Int))

303ae2e

Fix splitarg to correctly handle ::Int

ed199a6

cstjean force-pushed the pull-request/ebeefff7 branch from ba86652 to ed199a6 Compare June 21, 2017 16:38

cstjean added 3 commits June 23, 2017 21:01

splitkwargs for consistency

27adedb

Use only one definition for splitdef

1b4d042

Handle parametric definitions

fa94b4c

Allow travis failures on Julia nightly

00050a7

cstjean added 4 commits June 29, 2017 15:21

Detect slurp/splat in splitarg

0f88924

Test callable object syntax

b975c1a

Write combinearg

edfbd17

Fix combinearg for splatting arguments

cf8de2d

cstjean closed this Aug 9, 2017

willow-ahrens mentioned this pull request Jan 1, 2022

Fixes 165, Fixes 177. #178

Merged

Uh oh!

Support function return types in longdef/shortdef #35

Support function return types in longdef/shortdef #35

Uh oh!

Conversation

cstjean commented Mar 30, 2017

Uh oh!

cstjean commented Mar 31, 2017

Uh oh!

MikeInnes commented Apr 17, 2017

Uh oh!

cstjean commented Apr 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cstjean commented Apr 26, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MikeInnes May 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MikeInnes commented May 10, 2017

Uh oh!

cstjean commented May 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MikeInnes commented May 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cstjean commented May 22, 2017

Uh oh!

cstjean commented Jun 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cstjean commented Aug 9, 2017

Uh oh!

MikeInnes commented Aug 11, 2017

Uh oh!

cstjean commented Aug 11, 2017

Uh oh!

MikeInnes commented Aug 11, 2017

Uh oh!

Uh oh!

cstjean commented Apr 19, 2017 •

edited

Loading

MikeInnes May 10, 2017 •

edited

Loading

cstjean commented May 10, 2017 •

edited

Loading

MikeInnes commented May 15, 2017 •

edited

Loading

cstjean commented Jun 24, 2017 •

edited

Loading