Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

More careful ordering for @family tag #1563

Merged
merged 4 commits into from
Jan 9, 2024
Merged

More careful ordering for @family tag #1563

merged 4 commits into from
Jan 9, 2024

Conversation

krlmlr
Copy link
Member

@krlmlr krlmlr commented Dec 23, 2023

Needed for DBI: dbSendQuery() vs. dbSendQueryArrow() .

Test-first, the second commit afbabca shows the effect. Happy to revisit the implementation and/or switch the other tests to snapshots.

I suspect that the new order_c() should be used almost everywhere where we now have order() .

@krlmlr krlmlr changed the title Add test More careful ordering for @family tag Dec 23, 2023
@krlmlr krlmlr force-pushed the b-family-order branch 3 times, most recently from fac5576 to 2acfe95 Compare December 23, 2023 06:32
@krlmlr krlmlr requested a review from hadley December 23, 2023 06:33
@krlmlr krlmlr force-pushed the b-family-order branch 3 times, most recently from 95a6610 to afbabca Compare December 25, 2023 05:55
Copy link
Member

@hadley hadley left a comment

Choose a reason for hiding this comment

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

Looks fine to me. I can get in the next roxygen2 release if you add a news bullet.

@@ -123,3 +123,29 @@ test_that("custom family prefixes can be set", {

expect_match(out$get_value("seealso"), "^Custom prefix:")
})

test_that("careful ordering", {
# Can't use foo1 because it's used as class in another test
Copy link
Member

Choose a reason for hiding this comment

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

Is it an S4 class? It might need a on.exit(removeClass("foo1")) call.

Copy link
Member Author

Choose a reason for hiding this comment

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

No, unfortunately, #1564 does not change the behavior here.

Copy link
Member Author

Choose a reason for hiding this comment

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

Happy to adapt if #1566 or a better version gets in.

@hadley hadley mentioned this pull request Jan 2, 2024
25 tasks
@krlmlr
Copy link
Member Author

krlmlr commented Jan 2, 2024

Thanks, good to go from my end.

@krlmlr
Copy link
Member Author

krlmlr commented Jan 5, 2024

Done, finally.

@krlmlr krlmlr requested a review from hadley January 5, 2024 05:23
@hadley hadley merged commit f5e79d5 into main Jan 9, 2024
12 checks passed
@hadley hadley deleted the b-family-order branch January 9, 2024 03:41
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants