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

Remove ellipsis dependency and use rlang #112

Closed
wants to merge 2 commits into from
Closed

Remove ellipsis dependency and use rlang #112

wants to merge 2 commits into from

Conversation

olivroy
Copy link
Contributor

@olivroy olivroy commented Jun 26, 2024

Done!
Since ellipsis is deprecated and replaced by equivalent rlang functions https://rlang.r-lib.org/news/index.html#argument-intake-1-0-0

@olivroy olivroy changed the title Drop Ellipsis and use rlang Drop Ellipsis and use rlang+ fix doc Jun 26, 2024
@olivroy olivroy changed the title Drop Ellipsis and use rlang+ fix doc Drop ellipsis and use rlang+ fix doc Jul 23, 2024
Copy link
Member

@andrie andrie left a comment

Choose a reason for hiding this comment

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

Thank you for this fix.

The core contribution of pointing to ellipsis is great, and I'd like to accept this. But please can you respond to the two comments I made, and address these?

#' @keywords internal
"_PACKAGE"


# The following block is used by usethis to automatically manage
# roxygen namespace tags. Modify with care!
## usethis namespace: start
#' @importFrom assertthat assert_that is.string
Copy link
Member

Choose a reason for hiding this comment

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

Is there a reason that you moved these lines into the roxygen protected area?

NAMESPACE Outdated
@@ -19,6 +19,7 @@ export(sortable_js_capture_input)
export(sortable_options)
export(sortable_output)
export(update_bucket_list)
export(update_rank_list)
Copy link
Member

Choose a reason for hiding this comment

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

The function update_rank_list() is deliberately not exported because it doesn't accurately solve the problem it's supposed to solve.

@olivroy
Copy link
Contributor Author

olivroy commented Sep 5, 2024

Just undid the other unrelated changes!

@olivroy olivroy changed the title Drop ellipsis and use rlang+ fix doc Drop ellipsis and use rlang Sep 5, 2024
@CLAassistant
Copy link

CLAassistant commented Sep 5, 2024

CLA assistant check
All committers have signed the CLA.

@andrie
Copy link
Member

andrie commented Sep 5, 2024

Hmmm. I'm not sure what happened, but the current status of this PR seems to not include the original change to use ellipsis?

I'm almost done implementing your original intent into the main branch, and will push this soon.

Once that's done, I'll close this PR.

Many thanks for identifying the deprecated code and suggesting the fix.

@andrie andrie closed this in 0f84046 Sep 5, 2024
@olivroy
Copy link
Contributor Author

olivroy commented Sep 5, 2024

@andrie You should revert this commit 0f84046. It was okay before. The idea is to drop ellipsis and use rlang::check_dots_unnamed() instead.

54ba7c7 is correct

@olivroy olivroy changed the title Drop ellipsis and use rlang Remove ellipsis dependency and use rlang Sep 5, 2024
@andrie
Copy link
Member

andrie commented Sep 6, 2024

Many thanks for your help.

I'll revert from ellipsis to rlang

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