Skip to content

Forcibly retain label attribute#10

Merged
stopsack merged 7 commits intostopsack:devfrom
DavisVaughan:fix/dplyr
Jan 18, 2026
Merged

Forcibly retain label attribute#10
stopsack merged 7 commits intostopsack:devfrom
DavisVaughan:fix/dplyr

Conversation

@DavisVaughan
Copy link
Contributor

Hi there, we are working on the next version of dplyr and your package was flagged in our reverse dependency checks.

if_else() fundamentally returns a new vector, so you can't expect it to retain arbitrary attributes like label. I've tried forcibly retaining label through some if_else() calls, but I'm still seeing some failures I don't understand, and I think I'll need some assistance from you to figure these out.

If you really need label to stick around, the only way to do so is to create a new S3 class that retains it by implementing vctrs methods https://vctrs.r-lib.org/articles/s3-vector.html

dplyr will be released on January 31, 2026. If you could please send an update of your package to CRAN before then, that would help us out a lot! Thanks!

death = dplyr::if_else(dplyr::row_number() %in% 99:101, NA, death),
death = {
label <- attr(death, "label")
dplyr::if_else(dplyr::row_number() %in% 99:101, NA, death)
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
dplyr::if_else(dplyr::row_number() %in% 99:101, NA, death)
death <- dplyr::if_else(dplyr::row_number() %in% 99:101, NA, death)

-->
Test passed with 1 success 🎉.

Copy link
Owner

@stopsack stopsack left a comment

Choose a reason for hiding this comment

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

@DavisVaughan - I appreciate you reaching out. Happy to help. Is the issue you identified confined to the these unit tests? See inline comment.

@DavisVaughan
Copy link
Contributor Author

Ah yes that looks like it fixed it, my bad!

@stopsack
Copy link
Owner

Cool. I will leave the PR for you to close when appropriate. Please reach out if I can help otherwise.

@DavisVaughan
Copy link
Contributor Author

Well, it's still needs to be merged into rifttable in some way or another right? Because without it the next release of dplyr is going to break rifttable's tests. I just had the wrong patch the first time around.

@stopsack stopsack changed the base branch from main to dev January 18, 2026 18:36
@stopsack stopsack merged commit 58fcc27 into stopsack:dev Jan 18, 2026
2 checks passed
@stopsack
Copy link
Owner

stopsack commented Jan 18, 2026

Now I see. I can confirm that all units tests in {rifttable} pass, thanks to your PR, both with the latest version of {dplyr} from GitHub (commit 2be1ab6) and with v1.1.4. I will bring these changes to main and submit v0.7.2 of the package to CRAN. Thanks, @DavisVaughan!

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.

2 participants