-
Notifications
You must be signed in to change notification settings - Fork 12
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
Add slice_ functions #68
Conversation
R/dplyr_methods.R
Outdated
tibble::rowid_to_column(var = 'row_number___') |> | ||
dplyr::select(-everything(), row_number___, {{ .by }}) |> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot!
Sorry if I am being pedantic here, but it would be better to do select
first and rowid_to_column
later both from a memory and elegance perspective.
Another little feedback, a standard we use is to do (this applies to all new function calls)
@importFrom tibble ...
and not use
tibble:: ...
With these two changes I think the PR is good to go!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No problem, will do
@wvictor14 have a look at the github action; it is failing. Check maybe on your local CHECK and BiocCheck because here I have to "allow" github action each time, unfortunately. After you become a contributor, github action will be runnable by you in the future. |
@wvictor14 please add your authorship details here https://docs.google.com/spreadsheets/d/19XqhN3xAMekCJ-esAolzoWT6fttruSEermjIsrOFcoo/edit?usp=sharing |
@stemangiola I fixed the errors for CRAN check. There are two bioccheck errors that I am not sure best to address:
and ERROR: At least 80% of man pages documenting exported objects must have Also I realized that the option of inputting a tibble to |
No problem, that could be another "challenge"
Could you please give me minimal example ? |
This is what I mean
Not sure if there is a more straightforward way, or how to make it more robust. What do you think? |
Great! I left a last comment to address (Above) and then this PR is on its way! |
I think I got them all! |
Thanks a lot @wvictor14 for your PR! Well done. Now that you are an expert, if you want to translate your PR to the other tidy adapters No expectations of course. |
Hi I found this issue on tidyomics and thought it would be good experience to try
Let me know how it looks and if there's any changes needed
This adds the slice_ family: