Remove select.epi_df
#507
Merged
Remove select.epi_df
#507
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
`select.epi_df` looked weird: - `reclass(selected, attr(selected, "metadata"))` would only have made sense if grouped_df had a `select` that dropped our class but not our metadata, implemented in a way that called our own impls for `dplyr_extending` that would make that metadata actually be correct (in the case of renaming). - (`dplyr_reconstruct(selected, selected)` might have made sense, if `reclass` acted on something that shouldn't actually become an `epi_df`, decaying back to a non-`epi_df` only if needed.) So it seemed like it relied on `dplyr_extending` being almost, but not quite, sufficient. But it looks like `grouped_df` doesn't even have a `select` impl, so it's not interfering anyway! So it looks like we can just get rid of our own impl.
We need dplyr >= 1.0.8 for some dplyr_col_modify or something of the sort, used by the column modifications recorder used by epi_archive. (We also needed a more recent dplyr for if_any and if_all.) Change some tests to actually run under dplyr 1.0.8, which was more rigid about the types of things going into if_else.
|
Hi @dshemetov & @dsweber2, it'd be great if one of you could sanity-check this and/or verify this isn't breaking things downstream in exploration-tooling. |
dshemetov
approved these changes
Aug 9, 2024
|
Awesome, thanks for the quick review! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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).
(backwards-incompatible changes to the documented interface) are noted.
Collect the changes under the next release number (e.g. if you are on
1.7.2, then write your changes under the 1.8 heading).
process.
Change explanations for reviewer
select.epi_dflooked suspicious/weird without explanatory comments, and I don't want us to start basing other changes on it without in-source comments to explain the details... but now it turns outselect.epi_dfseems to be completely unnecessary now. (But at some point we needed patch: get select working with grouping #390... perhaps older dplyr versions resulted in a weird situation that no longer appears in more recent dplyr versions, but the fact that we needed it at some point makes me a bit nervous so:)select, so I didn't fully track down the story here. But in response, I've bumped our dependency a little bit to 1.0.8, and adjusted some failing tests to work on that version (which had a more rigidif_else).Magic GitHub syntax to mark associated Issue(s) as resolved when this is merged into the default branch