Skip to content

refactor!: use rlang::arg_match() in igraph.match.arg()#1165

Merged
aviator-app[bot] merged 4 commits intomainfrom
match-arg
Apr 8, 2024
Merged

refactor!: use rlang::arg_match() in igraph.match.arg()#1165
aviator-app[bot] merged 4 commits intomainfrom
match-arg

Conversation

@maelle
Copy link
Copy Markdown
Contributor

@maelle maelle commented Jan 29, 2024

Fix #1059

Breaking change

rlang::arg_match() does not allow partial matching.

Argument names

The several.ok and choices arguments are not used hence my respectively dropping/renaming them.

Wondering about case sensitivity

I see that tests fail if I try making this case sensitive, because of lines such as

rigraph/R/community.R

Lines 1336 to 1344 in 3626862

cluster_leiden <- function(graph, objective_function = c("CPM", "modularity"),
weights = NULL, resolution_parameter = 1, beta = 0.01,
initial_membership = NULL, n_iterations = 2, vertex_weights = NULL) {
ensure_igraph(graph)
# Parse objective function argument
objective_function <- igraph.match.arg(objective_function)
objective_function <- switch(objective_function,
"cpm" = 0,

The usage of tolower() also means the error message is not aligned with the documentation. Should I work on aligning the usage of values in code with the usage of values in documentation? For instance using "CPM" not "cpm" in the switch()call. However maybe there are people who use "cpm" in their function calls, which would currently work, so that might mean a breaking change.

@maelle maelle added the upkeep maintenance, infrastructure, and similar label Jan 29, 2024
@maelle maelle requested a review from krlmlr January 29, 2024 08:22
@aviator-app
Copy link
Copy Markdown
Contributor

aviator-app bot commented Jan 29, 2024

Current Aviator status

Aviator will automatically update this comment as the status of the PR changes.
Comment /aviator refresh to force Aviator to re-examine your PR (or learn about other /aviator commands).

This PR was merged using Aviator.


See the real-time status of this PR on the Aviator webapp.
Use the Aviator Chrome Extension to see the status of your PR within GitHub.

@maelle maelle changed the title refactor: use rlang::arg_match() in igraph.match.arg() refactor!: use rlang::arg_match() in igraph.match.arg() Jan 29, 2024
@maelle
Copy link
Copy Markdown
Contributor Author

maelle commented Mar 22, 2024

Should we run revdeps with this?

@krlmlr
Copy link
Copy Markdown
Contributor

krlmlr commented Mar 22, 2024

Starting checks now.

@maelle
Copy link
Copy Markdown
Contributor Author

maelle commented Mar 25, 2024

The revdeps checks looked good but I had forgotten to rebase this branch, I am sorry. 😞

@krlmlr krlmlr marked this pull request as ready for review April 2, 2024 12:59
Copy link
Copy Markdown
Contributor

@krlmlr krlmlr left a comment

Choose a reason for hiding this comment

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

revdepchecks are good, thanks!

@aviator-app aviator-app bot added the blocked label Apr 2, 2024
@aviator-app
Copy link
Copy Markdown
Contributor

aviator-app bot commented Apr 2, 2024

This pull request failed to merge: some CI status(es) failed. Once the issues are resolved, remove the blocked label and re-queue the pull request. Note that the pull request will be automatically re-queued if it has the mergequeue label.

Failed CI(s): Check windows-latest (release)

@maelle maelle removed the blocked label Apr 8, 2024
@aviator-app aviator-app bot added the blocked label Apr 8, 2024
@aviator-app
Copy link
Copy Markdown
Contributor

aviator-app bot commented Apr 8, 2024

This pull request failed to merge: some CI status(es) failed. Once the issues are resolved, remove the blocked label and re-queue the pull request. Note that the pull request will be automatically re-queued if it has the mergequeue label.

Failed CI(s): Check windows-latest (release)

@maelle maelle added upkeep maintenance, infrastructure, and similar mergequeue blocked and removed upkeep maintenance, infrastructure, and similar mergequeue blocked labels Apr 8, 2024
@maelle maelle removed the blocked label Apr 8, 2024
@aviator-app aviator-app bot merged commit b963b86 into main Apr 8, 2024
@aviator-app aviator-app bot deleted the match-arg branch April 8, 2024 12:17
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 9, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

upkeep maintenance, infrastructure, and similar

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Use rlang::arg_match() in igraph.match.arg()

2 participants