Skip to content

Conversation

ChloeYou
Copy link
Contributor

Closes #45.

Should this also be merged into the dev branch?

@ChloeYou ChloeYou requested a review from brookslogan June 13, 2022 19:31
@ChloeYou ChloeYou requested a review from dajmcdon as a code owner June 13, 2022 19:31
@dshemetov dshemetov changed the base branch from main to frosting June 13, 2022 22:55
@dshemetov
Copy link
Contributor

dshemetov commented Jun 13, 2022

I just switched this to merge into the frosting branch. If we merge #49 in first (where main is merged into frosting), then the long commit log should disappear here.

@brookslogan
Copy link
Contributor

Right, this PR probably shouldn't be the one to merge main into frosting. Taking a look at #49 to see if it can be merged in first.

@brookslogan
Copy link
Contributor

Also, I assume the merge conflicts GitHub is flagging now might automatically be resolved if #49 is merged.

@dshemetov dshemetov changed the base branch from frosting to main June 14, 2022 22:53
@dshemetov dshemetov changed the base branch from main to frosting June 14, 2022 22:53
@ChloeYou ChloeYou requested a review from brookslogan June 15, 2022 18:24
While `dplyr` appears to automagically provide its own `across` inside
`group_by`, we still want to explicitly use `dplyr::` to
- satisfy package checks,
- continue to work if `dplyr` removes this magic (e.g., if the maintainers don't
  like this magic ignoring any user-defined/attached non-`dplyr` `across`
  function), and
- be clear to code readers&editors where the function comes from.
@brookslogan brookslogan self-requested a review June 16, 2022 11:03
Copy link
Contributor

@brookslogan brookslogan left a comment

Choose a reason for hiding this comment

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

Looks good to resolve #45. I have some other questions and comments about how the function is supposed to work, but I will add them to a separate issue.

@brookslogan brookslogan self-requested a review June 16, 2022 11:07
@brookslogan brookslogan merged commit 9dd8c8a into cmu-delphi:frosting Jun 16, 2022
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.

get_test_data() should return an ungrouped epi_df

4 participants