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

remove soft deprecated dplyr-src funs #1000

Merged
merged 4 commits into from
May 22, 2022

Conversation

pawelru
Copy link
Contributor

@pawelru pawelru commented May 20, 2022

close #999

I have found many other calls to those functions but I decided to leave them - here goes my explanation:

  • dm_get_src()
    image

    • R/ - only deprecated function itself
    • scratch - I have left your notes untouched
    • tests/ - as you soft-deprecated then that means that you still expect it to work - would be good to keep that check
  • tbl()
    image

    • I left demo/ directory as it is as I assume it's for demoes that already happened so this code is unlikely to be executed again. If yes - should be done on the current version at that point in time.
    • R/ - only comments and deprecated function itself
    • tests/ - same as above
  • src_tbls()
    image

    • demo/ - as above
    • I think that the last occurence is intended to call dplyr::src_tbls (would be good to prefix though) - please confirm
  • copy_to()
    image

    • R/ - same as above
    • tests/ - same as above
    • vignettes/ - here I probably need your help with assessment. Should it stay or not?

Copy link
Collaborator

@krlmlr krlmlr left a comment

Choose a reason for hiding this comment

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

Thanks a lot! Is there a reason to prefer [[ over $ ?

@@ -265,7 +265,7 @@ prepare_dm_for_flatten <- function(dm, tables, gotta_rename) {
red_dm <-
dm_reset_all_filters(dm) %>%
dm_select_tbl(!!!tables)
# Only need to compute `tbl(dm, start)`, `dm_apply_filters()` not necessary
# Only need to compute `dm[[start]]`, `dm_apply_filters()` not necessary
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
# Only need to compute `dm[[start]]`, `dm_apply_filters()` not necessary
# Only need to compute `dm$start`, `dm_apply_filters()` not necessary

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks a lot! Is there a reason to prefer [[ over $ ?

Yes. [[ works with variables storing a string wheras $ doesn't. E.g. here.

Is start a variable here (that stores the string) or it is a string (which then should be a "start")? Current code indicates that it's a variable.

If it's a variable then it needs to stay as dm[[start]] and if it a string then it could be changed into dm$start.

vignettes/tech-dm-join.Rmd Outdated Show resolved Hide resolved
@krlmlr
Copy link
Collaborator

krlmlr commented May 20, 2022

The demos are part of R CMD check, we will catch when things start to fail. But then they are only of historic interest, so if this happens we could prune all except the most recent one.

@krlmlr
Copy link
Collaborator

krlmlr commented May 20, 2022

For the vignettes we'd want to look for idiomatic replacements. Happy to provide guidance as needed, pinging @TSchiefer and @moodymudskipper too.

@pawelru
Copy link
Contributor Author

pawelru commented May 20, 2022

Thanks a lot! Is there a reason to prefer [[ over $ ?

Nothing really strong. To me, [[ deals better when list element name contains a "$". You would then have to add some back-ticks or quite which doesn't look good right after $. But this is rather my personal preference - you are the owner so I would do what you prefer

> x <- list("a$b" = 1, a = 2, b = 3)
> x$a$b
Error in x$a$b : $ operator is invalid for atomic vectors
> x$`a$b`
[1] 1
> x$"a$b"
[1] 1
> x[["a$b"]]
[1] 1

R/flatten.R Outdated Show resolved Hide resolved
Copy link
Collaborator

@krlmlr krlmlr left a comment

Choose a reason for hiding this comment

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

Thanks! Let's get this merged so that future PRs don't require confirmation to run workflows.

@krlmlr krlmlr merged commit 6d9a2ad into cynkra:main May 22, 2022
@krlmlr
Copy link
Collaborator

krlmlr commented May 22, 2022

Thanks!

@pawelru pawelru deleted the 999_rm_soft_depr_funs branch October 5, 2022 08:33
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 6, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Remove all calls of soft deprecated functionalities
2 participants