Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Inaccurate documentation for name argument of count() #5958

Closed
sfirke opened this issue Jul 27, 2021 · 14 comments · Fixed by #6352
Closed

Inaccurate documentation for name argument of count() #5958

sfirke opened this issue Jul 27, 2021 · 14 comments · Fixed by #6352

Comments

@sfirke
Copy link
Contributor

sfirke commented Jul 27, 2021

The documentation for count() says of the argument name:

If omitted, it will default to n. If there's already a column called n, it will error, and require you to specify the name.

But when I try calling count on a variable named n, that's not what happens - it succeeds with a warning:

mtcars %>% group_by(cyl, am) %>% summarise(n = n(), .groups = "drop") %>% count(cyl, n)
Storing counts in `nn`, as `n` already present in input
i Use `name = "new_name"` to pick a new name.
# A tibble: 6 x 3
    cyl     n    nn
  <dbl> <int> <int>
1     4     3     1
2     4     8     1
3     6     3     1
4     6     4     1
5     8     2     1
6     8    12     1

This is trivial, I think, and I like the current behavior of count() warning instead of erroring - I'm just noting the documentation mismatch.

@hadley
Copy link
Member

hadley commented Sep 16, 2021

Do you want to do a PR?

@hadley hadley added the help wanted ❤️ we'd love your help! label Sep 16, 2021
@sfirke
Copy link
Contributor Author

sfirke commented Sep 17, 2021

This would be a great first pull request for someone, I think -- I will leave it for a newcomer to the project, if that's okay? If anyone replies to this thread to claim it and wants help, I can answer basic Qs. And if it's still unclaimed in a few months I'll do a PR.

@KittJonathan
Copy link

Hello there, is this an issue a newbie like me can work on ? I'm a great user of R, tidyverse & so on, but I'm new to dev work ...

@sfirke
Copy link
Contributor Author

sfirke commented Oct 1, 2021

Yep this would be a good one to learn the 101 of the dev workflow! You can read up on it more but in short, you'd: fork this repository, clone your forked repo to your computer, edit these lines

dplyr/R/count-tally.R

Lines 26 to 27 in 0445420

#' If omitted, it will default to `n`. If there's already a column called `n`,
#' it will error, and require you to specify the name.
to describe the actual behavior, run document() from the devtools package to regenerate the new documentation for count(), push your changes back to your forked repo, and submit a pull request to this one asking for your changes to be merged in.

@KittJonathan
Copy link

Great, thanks for the details ! Will look into this ASAP.

@sfirke
Copy link
Contributor Author

sfirke commented Oct 1, 2021

(to be clear: I'm not a developer on this repo, and I can't speak for Hadley et al. Just clarifying that. But I do maintain other R packages so am pretty confident in what I described, though there might be some additional details that they might want).

@KittJonathan
Copy link

Made the changes in the roxygen comments, but when I run devtools:document() I get the following error :

Erreur dans new_environment() (depuis context.R#120) :
impossible de trouver la fonction "new_environment"
De plus : Messages d'avis :
1: Need rlang >= 0.4.11.9001 but loaded version is 0.4.11
2: Dans loadNamespace(i, c(lib.loc, .libPaths()), versionCheck = vI[[i]]) :
le package ‘rlang’ 0.4.11 est déjà chargé, mais >= 0.4.11.9001 est requis

Tried updating rlang but it failed, can't install it again using remotes ... Am I missing something here ?

@sfirke
Copy link
Contributor Author

sfirke commented Oct 4, 2021

Sorry to hear that. It looks like you installed a dev version package from GitHub that has changes that require the dev version of rlang .9001. This is a situation I find myself in sometimes, where I have a mix of released package versions from CRAN and dev package versions from Github, and it becomes a catch-22 where I need a dev version of rlang or other core tidyverse package but I can't install it because I don't have it. I suspect it's a Windows thing because I don't hear about it much from others.

Anyway, this is separate from anything related to this dplyr issue. I would suggest trying to do it all with released package versions if you can. To resolve your situation, you might try remove.packages() to get rid of packages, and install.packages() to get the CRAN versions back again, restarting RStudio whenever you get stuck.

@KittJonathan
Copy link

Thanks for the tips, but unfortunately nothing works. I upgraded R, reinstalled the packages, and I still get this error message when trying to run devtools:document()

Need rlang >= 0.4.11.9001 but loaded version is 0.4.11

Can't install rlang dev using remotes or install_github either.
I'll maybe just stick to writing code and not try dev :-)

@sfirke
Copy link
Contributor Author

sfirke commented Oct 5, 2021

Bummer, sorry to hear that. Did you install a package from GitHub? I think you could avoid that error by using all CRAN packages. rlang on CRAN is 0.4.11. If you have a package that's requiring the one ending in .9000 it makes me think you installed a package from GitHub.

But, if you don't want to spend more time slogging through this, I don't blame you. This kind of hassle is the worst part of package development 😔

@KittJonathan
Copy link

The DESCRIPTION file for dplyr has

Imports:
rlang (>= 0.4.11.9001)

Maybe that's the problem, since I can't install the dev version for rlang ...

@sfirke
Copy link
Contributor Author

sfirke commented Oct 5, 2021

Ah yes of course, you're working with the dev version of dplyr, because that's the package we're trying to edit 🤦 That's it, sorry. Maybe you could clone the repo but not install it, and do everything with CRAN packages -- unless devtools::document() would check for the fact that you don't have the dev version of rlang, in which case you'd be forced to try to install all the dev versions. Which is what got you stuck. If that doesn't work, this might be impassable.

I don't run into this bad loop with rlang when I'm using Linux, and I suspect Mac is better too, if either is option for you (if you're already using one of those platforms, disregard).

@KittJonathan
Copy link

Ok thanks, tried it without success. That'll be an argument for work to have a Linux setup :)
Thanks for your help, will keep an eye on issues I might help with !

@DOH-JPD2303
Copy link

See #6288.

hadley added a commit that referenced this issue Jul 21, 2022
hadley added a commit that referenced this issue Aug 2, 2022
* Document all overridden functions in setops topic, instead of relying on re-exports.
* Tweak topic titles and reorganise reference index. Fixes #6017.
* Better description of `count(name =)`. Fixes #5958.
* Improve documentation of ranking functions. Fixes #6233. Fixes #5943.
* Don't include "methods" found in base packages. Fixes #6326
* Polish across docs. Fixes #6205
# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants