search

igraph / rigraph

igraph R package
https://r.igraph.org
557 stars 202 forks source link

Add ellipsis #1045

Open krlmlr opened 11 months ago

krlmlr commented 11 months ago

https://github.com/igraph/rigraph/pull/1028#discussion_r1438319072

krlmlr commented 11 months ago

@ntamas: Works like a charm, idempotent, fd74ecf7ad17bd741578cb1908da6fb190989798 is an example of an added * . We'll wait for those to be available in the C core.

What does it take to add a check_dots_empty() call when a * is present in PARAMS?

ntamas commented 11 months ago

check_dots_empty() added in Stimulus 0.21.1.

krlmlr commented 11 months ago

Works as expected, in 894015601109f56718977aa85d4c02dacd4679e9.

szhorvat commented 11 months ago

check_dots_empty()

Is this function waiting to be implemented? Let me know when that's done and I'll continue with #1028 afterwards.

szhorvat commented 11 months ago

Ah, so this is an rlang thing. Should it be called as rlang::check_dots_empty()? Or what is the solution to the following?

> k_shortest_paths(make_ring(5),1,2)
Error in check_dots_empty() : could not find function "check_dots_empty"
szhorvat commented 11 months ago

I guess I need to add #' @importFrom rlang check_dots_empty?

krlmlr commented 11 months ago

Thanks, missed that. Waiting for * to be configured in the core, eager to roll out here and run revdepchecks.

krlmlr commented 11 months ago

One more thing: in my book, the weights argument should always come after the ellipsis (if it exists, which I assume it will for most functions). This is contrary to https://github.com/igraph/igraph/wiki/Guidelines-for-function-argument-ordering .

https://github.com/igraph/rigraph/pull/1028#discussion_r1438727059

ntamas commented 11 months ago

The guidelines you cited above are for the C core. High-level interfaces are free to come up with their own guidelines in order to make the generated functions fit better with the conventions of the high-level language.

krlmlr commented 11 months ago

Could Stimulus take care of that for the autogenerated code? Do we agree it's a good idea?

ntamas commented 11 months ago

It could, but I'm not sure whether it's in the scope of the project.

I'd say that in the long term Stimulus should probably be refactored such that the repo contains the core only and the individual language-specific generators are in the repositories of the higher-level interfaces (with some kind of a plug-in system using Python's standard entry points from importlib). If that happens, you could do whatever you want with the R code generator as it would be entirely within your domain.