Skip to content
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

added support for @separate_rows #72

Merged
merged 6 commits into from
Dec 19, 2023
Merged

Conversation

drizk1
Copy link
Member

@drizk1 drizk1 commented Dec 17, 2023

I removed the macro component for grouped dataframes because I could not think of a scenario to group_by before separating rows that would change the outcome. I'm happy to add support to the function if that is useful.

@kdpsingh
Copy link
Member

kdpsingh commented Dec 17, 2023

Awesome! To your question about grouping, I'd need to look and see how it works in dplyr/tidyr. My guess is that we need to "respect" grouping.

In other words, my guess is that if a grouped data frame is used, then we would ungroup it, apply the separate_rows transformation, and then regroup it. But I could be wrong.

We already do this for a handful of other macros I think, so it should be easy to implement if needed. Happy to help.

I would first check the behavior in R.

@drizk1
Copy link
Member Author

drizk1 commented Dec 17, 2023

So i changed the function to now check if it is grouped, ungroup if it is, apply logic to ungrouped df, then regroup at the end. So now the macro can be used in a grouped chain / after a @group_by. thx for the tip!

I played around with a small df in R, and couldn't seem to elicit changes to the separating based on grouping or not, so I think this should be ok.

@kdpsingh
Copy link
Member

Thanks! I'll review and plan to merge soon.

@kdpsingh kdpsingh merged commit 9c42273 into TidierOrg:main Dec 19, 2023
3 checks passed
@drizk1 drizk1 deleted the add-@separate_rows branch December 19, 2023 12:33
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