-
Notifications
You must be signed in to change notification settings - Fork 8
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
Djm/as tibble attr #471
Djm/as tibble attr #471
Conversation
Merge branch 'djm/as_tibble-attr' of https://github.com/cmu-delphi/epiprocess into djm/as_tibble-attr # Conflicts: # R/methods-epi_df.R
b363084
to
d1d47c5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approach looks nice, we get to keep sane behavior for users that take it as_tibble()
by its name. I added a bit of roxygen, NEWS.md, and conditional attr printing to make it less mysterious.
Question: how do you want propagation for this attribute to work? It looks like there is some amount of propagation already, e.g., surviving through mutate()
, but it does not survive via |> group_by(geo_value) |> ungroup()
(unlike our metadata). (And there are still other cases (e.g., #250) where our metadata / epi_df
ness doesn't survive when we'd want it to.) If you want it to be hardier, then it probably belongs within metadata
+ a double-check of whether reclass()
and dplyr_reconstruct.epi_df()
do indeed propagate it then.
My thinking on grouping: since I suppose an external user could want this functionality, but providing it seems likely to be very low on the pile of issues until someone asks. |
I didn't mean that we shouldn't drop the grouping with |
Oh, I see. Yes, I'm hopeful that there's no grouping on internally that will screw me up. But good to have a record here in case it does! |
Checklist
Please:
PR).
brookslogan, nmdefries.
DESCRIPTION
. Always incrementthe patch version number (the third number), unless you are making a
release PR from dev to main, in which case increment the minor version
number (the second number).
process.
Change explanations for reviewer
Magic GitHub syntax to mark associated Issue(s) as resolved when this is merged into the default branch
as_tibble()
to drop the class? #467