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

Allow for keyword arguments #323

Merged
merged 23 commits into from
Oct 19, 2022
Merged

Conversation

pdeffebach
Copy link
Collaborator

This just adds keyword arguments for calls of the form @subset(df, :a; view = true), rather than the multi-line version

@subset df begin 
    :a 
end

I have added this for @subset but not others yet.

@pdeffebach
Copy link
Collaborator Author

@jkrumbiegel can I get a quick check on the implementation? It borrows from DataFrameMacros.jl.

With your approval I will add the same structure to everything else.

@jkrumbiegel
Copy link
Contributor

Yeah looks ok from a quick glance. I still haven't decided how keyword arguments could work for non-parentheses style.

@pdeffebach
Copy link
Collaborator Author

I guess the current code isn't very DRY, but each 4-line *_helper function is unique and it seems overly abstract to make an outer function right now.

Will plug away with tests.

@pdeffebach
Copy link
Collaborator Author

I think I have the pipeline figured out for how to process arguments. I've written it for @rtransform but need to port it to the rest of the macros.

@bkamins
Copy link
Member

bkamins commented Sep 15, 2022

Nice!

@pdeffebach
Copy link
Collaborator Author

Okay! I think this is all done @bkamins @nalimilan @jkrumbiegel

Please take a look!

t = (fun_to_vec(ex; no_dest=true, outer_flags=outer_flags) for ex in exprs)
quote
$subset($x, $(t...); skipmissing=true)
$subset($x, $(t...); (skipmissing = true,)..., $(kw...))
Copy link
Member

Choose a reason for hiding this comment

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

is passing skipmissing explicitly tested? Why does skipmissing need to be in a tuple and later ...?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It is explicitely tested for throwing an error. Every keyword argument is tested.

I do not know why exactly I need a tuple and splatting, but by omitting that I get

julia> @subset(df, :x .== 1; skipmissing = false)
ERROR: syntax: keyword argument "skipmissing" repeated in call to "DataFrames.subset" around /home/peterwd/Documents/Development/DataFramesMeta/src/macros.jl:765
Stacktrace:
 [1] top-level scope
   @ REPL[8]:1

so it seems necessary.

Copy link
Member

Choose a reason for hiding this comment

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

ah - OK. So that is why it is needed. If you pass it explicitly uniqueness is tested, if you pass collections they are merged. This is strange, as this is not required in other contexts:

julia> (;a=1, [:a => 2]...)
(a = 2,)

(and you get merging without having to wrap the first in a tuple.

src/macros.jl Outdated Show resolved Hide resolved
src/macros.jl Show resolved Hide resolved
@pdeffebach
Copy link
Collaborator Author

Thanks!

@pdeffebach
Copy link
Collaborator Author

Okay I'm ready to merge this. There will always be more docs to add but I think this is now pretty comprehensive.

@pdeffebach pdeffebach merged commit c63d85f into JuliaData:master Oct 19, 2022
@pdeffebach pdeffebach deleted the keyword_arguments branch October 19, 2022 12:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants