-
-
Notifications
You must be signed in to change notification settings - Fork 206
feat: change constructors to use explicit arguments #2467
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
feat: change constructors to use explicit arguments #2467
Conversation
Update constructor functions to accept explicit arguments instead of just `...`, matching their corresponding `make_*` and `sample_*` function signatures. This improves IDE autocomplete and makes the API clearer. Exceptions: - `directed_graph()` and `undirected_graph()` kept as `...` since they work with `make_()` where arguments can be passed in various orders - `tree()` and `from_literal()` kept as `...` with .lazy = TRUE due to their special lazy evaluation behavior Co-authored-by: krlmlr <[email protected]>
Co-authored-by: krlmlr <[email protected]>
R/make.R
Outdated
| mode = c("in", "out", "mutual", "undirected"), | ||
| center = 1 | ||
| ) { | ||
| constructor_spec(make_wheel, n, ..., mode = mode, center = center) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| constructor_spec(make_wheel, n, ..., mode = mode, center = center) | |
| check_dots_empty() | |
| constructor_spec(make_wheel, n, mode = mode, center = center) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Applied in c246631. Added check_dots_empty() to wheel() and other constructors with ... parameter (growing, bipartite_gnm, bipartite_gnp).
|
@copilot: Apply feedback, fix errors seen with |
- Add check_dots_empty() to constructors with ... parameter (wheel, growing, bipartite_gnm, bipartite_gnp) - Revert from_edgelist() and from_data_frame() to use ... for flexibility with graph_() - Fix documentation warnings by adding @inheritParams rlang::args_dots_empty - Remove @param ... from constructors that no longer have ... parameter - Update sample_() example to remove pattern that no longer works Co-authored-by: krlmlr <[email protected]>
Fixed in c246631. All devtools::check() errors and warnings resolved:
Status: 0 errors ✔ | 0 warnings ✔ | 0 notes ✔ Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
krlmlr
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot: Merge main, adhere to feedback. Always use named arguments for forwarding.
| #' @export | ||
| growing <- function(...) constructor_spec(sample_growing, ...) | ||
| growing <- function(n, m = 1, ..., directed = TRUE, citation = FALSE) { | ||
| check_dots_empty() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add the check to the sampler as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added in 269d1bb. Added check_dots_empty() to sample_growing().
…uctors-to-use-explicit-arguments
- Merge main branch (includes version bump and modularity fix) - Add check_dots_empty() to sample_growing() as requested - Change all constructor_spec calls to use named arguments for consistency and clarity - This ensures better code readability and makes argument forwarding explicit All constructors now use named arguments when calling constructor_spec: - R/make.R: star, full_graph, ring, atlas, chordal_ring, circulant, line_graph, de_bruijn_graph, kautz_graph, from_prufer, bipartite_graph, full_bipartite_graph, full_multipartite, turan, full_citation_graph - R/games.R: gnp, gnm, growing, grg, smallworld, last_cit, cit_types, cit_cit_types, bipartite_gnm, bipartite_gnp, sbm, hierarchical_sbm, dot_product - R/adjacency.R: from_adjacency Co-authored-by: krlmlr <[email protected]>
Completed in 269d1bb:
All 40+ constructors now explicitly name all forwarded arguments for clarity and maintainability. Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
|
@schochastics: Good to merge? |
|
Do we even need to export the constructors? In #2500, there is a collision with |
schochastics
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
I assume we cannot simple remove them since they were available before? I might have to think more about it but I currently dont see where these could be more handy than the corresponding make functions? |
Change constructors to use explicit arguments
Summary
Updated constructor functions to accept explicit arguments instead of just
..., matching their correspondingmake_*andsample_*function signatures. This improves IDE autocomplete and makes the API clearer.Latest changes
check_dots_empty()tosample_growing()- ensures consistency with the constructorconstructor_spec():constructor_spec(make_wheel, n = n, mode = mode, center = center)instead ofconstructor_spec(make_wheel, n, mode = mode, center = center)Testing
✅ All tests pass: 7,418 passed, 2 skipped (unrelated), 0 failures
✅
devtools::check()passes: 0 errors, 0 warnings, 0 notesOriginal prompt
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.