Skip to content

Add log to dplyr function ungroup() #33

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

Merged
merged 21 commits into from
Nov 10, 2019
Merged

Add log to dplyr function ungroup() #33

merged 21 commits into from
Nov 10, 2019

Conversation

damianooldoni
Copy link
Contributor

This PR aims to solve #32.

  1. I opted for log test: ungroup: no grouping variables left.
  2. New R file ungroup.R added to ./R directory. Another way to work would be to add function ungroup() in file ./R/group_by.R.
  3. I added a related test file: test_ungroup.R
  4. I slightly improved other tests as well. For example: no need to load library in each test file.
  5. README is also updated by adding ungroup() at the end of example about grouping.

Travis says yes and pkg documentation seems ok 👍
Have a deep look and let me know. Especially point 2 seems something you, as maintainer, has the last word about.

Thanks for writing, updating and maintaining this wonderful package.

Copy link
Owner

@elbersb elbersb left a comment

Choose a reason for hiding this comment

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

Thanks!
I added a few comments. As you've suggested I'd prefer the ungroup function be part of group_by. (Same for the tests)
As for the tests files, I actually like having the library calls on top, because that makes debugging them easier. Could you revert? In general, also best to keep things in separate PRs, so it's easier to review :)
Also feel free to add a new bullet point in NEWS.md.

DESCRIPTION Outdated
@@ -1,9 +1,13 @@
Package: tidylog
Type: Package
Title: Logging for 'dplyr' and 'tidyr' Functions
Title: Logging for 'dplyr' and 'tidyr' functions
Copy link
Owner

Choose a reason for hiding this comment

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

not a typo

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done: 3e70eea

DESCRIPTION Outdated
person("Benjamin", "Elbers", email = "[email protected]",
role = c("aut", "cre"), comment = c(ORCID = "0000-0001-5392-3448")),
person("Damiano", "Oldoni", email = "[email protected]",
role = c("aut", "ctb"), comment = c(ORCID = "0000-0003-3445-7562"))
Copy link
Owner

Choose a reason for hiding this comment

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

please use just "ctb" here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am very sorry: I am definitely not the author! A copy paste error.
Solved here: baae929

README.Rmd Outdated
```
Here, it might have been accidental that the last `filter` command had no effect.
Here, it might have been accidental that the `filter` command had no effect.
Copy link
Owner

Choose a reason for hiding this comment

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

please leave as is, or change to "second"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, you are right: it refers to the fact that there are two filters, indeed.
Solved here: 39c624f

@damianooldoni
Copy link
Contributor Author

I think I solved all requested changes. Please, give a look again. Thanks.

@elbersb
Copy link
Owner

elbersb commented Oct 31, 2019

Thanks, that's great.
Unfortunately, I had just had another idea, and that is to simply add this functionality to the existing group_by function. This has two advantages: 1) it takes care of the case group_by(mtcars), which prints something ugly right now, and 2) makes the package more future proof in case the ... argument in ungroup is ever taken seriously. (see, e.g., tidyverse/dplyr#3760)

If you'd like to have another go at this, feel free -- if not, I'll merge and then implement this.

@damianooldoni
Copy link
Contributor Author

I like your idea. I gave it a try. Let me know. Thanks.

@elbersb elbersb merged commit 3b95ce1 into elbersb:master Nov 10, 2019
@elbersb
Copy link
Owner

elbersb commented Nov 10, 2019

Thanks!

@elbersb elbersb mentioned this pull request Nov 10, 2019
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