Skip to content

Fix roxygen2 issue with all/all.equal ambiguity#339

Open
MichaelChirico wants to merge 3 commits into
insightsengineering:mainfrom
MichaelChirico:patch-1
Open

Fix roxygen2 issue with all/all.equal ambiguity#339
MichaelChirico wants to merge 3 commits into
insightsengineering:mainfrom
MichaelChirico:patch-1

Conversation

@MichaelChirico

Copy link
Copy Markdown

You are affected by r-lib/roxygen2#1587. Came across your package while looking for a solution, so sharing what I found.

@github-actions

github-actions Bot commented Oct 13, 2024

Copy link
Copy Markdown
Contributor

✅ All contributors have signed the CLA
Posted by the CLA Assistant Lite bot.

@MichaelChirico

Copy link
Copy Markdown
Author

I have read the CLA Document and I hereby sign the CLA

@MichaelChirico

Copy link
Copy Markdown
Author

recheck

@averissimo

Copy link
Copy Markdown
Contributor

Thanks for the PR

I don't think this roxygen2 issue affects this package as the all.equal method is only meant to be available during tests.

We achieve that by registering the S3 method in a {testthat} helper.

if (testthat::is_testing()) {
registerS3method("all.equal", "join_keys", all.equal.join_keys)
}

@gogonzo do you want to revisit the export of this method?

Initially, we were hoping waldo would merge r-lib/waldo#186 as well as moving to {testthat} v3, but given that PR is almost 1 year old that seems unlikely.

@gogonzo

gogonzo commented Oct 14, 2024

Copy link
Copy Markdown
Contributor

I don't think either that this affects us.

@MichaelChirico

Copy link
Copy Markdown
Author

to clarify I'm referring specifically to this:

\method{all}{equal.join_keys}(target, current, ...)

@llrs-roche

Copy link
Copy Markdown
Contributor

From previous experience registering the right S3 method helps with the dispatch, even if it is not exported (no exportMethods(all) or exportMethods(all.equal)) it must be registered to work well. I'm not familiar enough with join_keys and join_key_set but it is important on this context.

I also consider this very minimal (see the last commit) and without issues on a local check.

@m7pr

m7pr commented Nov 8, 2024

Copy link
Copy Markdown
Contributor

r-lib/waldo#186 got merged

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.

5 participants