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

Rename group/group_by arguments into by #502

Merged
merged 27 commits into from
May 18, 2024
Merged

Rename group/group_by arguments into by #502

merged 27 commits into from
May 18, 2024

Conversation

strengejacke
Copy link
Member

@strengejacke strengejacke changed the title Rename group/group_by arguments into by. Rename group/group_by arguments into by May 16, 2024
NEWS.md Outdated Show resolved Hide resolved
R/data_partition.R Outdated Show resolved Hide resolved
@strengejacke
Copy link
Member Author

hm, this error seems to be new, but unrelated to this PR. Why does this encoding-error suddenly occur?

https://github.com/easystats/datawizard/actions/runs/9109670481/job/25043053199?pr=502

@etiennebacher
Copy link
Member

It might be due to an xfun update: yihui/xfun#85

NEWS.md Outdated Show resolved Hide resolved
@strengejacke
Copy link
Member Author

We have some aliases, we should think about when to remove them?

#' @rdname text_format
#' @export
format_text <- text_format
#' @rdname recode_values
#' @export
change_code <- recode_values

@etiennebacher
Copy link
Member

We have some aliases, we should think about when to remove them?

Yes, we could also address #265 at the same time

NEWS.md Outdated Show resolved Hide resolved
strengejacke and others added 2 commits May 17, 2024 13:10
@etiennebacher
Copy link
Member

Sorry @strengejacke I meant that we could address the text aliases you mentioned and the aliases mentioned in #265 at the same time but in another PR so that we keep this one strictly about renaming args. Sorry for the confusion 😬

@strengejacke
Copy link
Member Author

I hope we don't need to add @rdname tags to the deprecated functions, but I fear, it must be... Let's see what the checks say.

@strengejacke
Copy link
Member Author

@etiennebacher Why do we have this error for the selection-syntax vignette now?
https://github.com/easystats/datawizard/actions/runs/9127735513/job/25098644747?pr=502

@etiennebacher
Copy link
Member

Sorry @strengejacke I meant that we could address the text aliases you mentioned and the aliases mentioned in #265 at the same time but in another PR so that we keep this one strictly about renaming args. Sorry for the confusion 😬

@strengejacke have you seen this?

@strengejacke
Copy link
Member Author

Sorry @strengejacke I meant that we could address the text aliases you mentioned and the aliases mentioned in #265 at the same time but in another PR so that we keep this one strictly about renaming args. Sorry for the confusion 😬

Yeah, there are quite some files that have been changed... 🤣 But I think it's ok.

@strengejacke
Copy link
Member Author

Sorry @strengejacke I meant that we could address the text aliases you mentioned and the aliases mentioned in #265 at the same time but in another PR so that we keep this one strictly about renaming args. Sorry for the confusion 😬

@strengejacke have you seen this?

I'm too unfamiliar with git to know how to split commits into separate PRs... 😬

@etiennebacher
Copy link
Member

I'd really like to keep those separated so that we can point to a clear PR if necessary later, I can try to split this into two PRs a bit later

@strengejacke
Copy link
Member Author

Actually, this PR can be merged. I'm not sure it's worth the effort splitting this PR (I just saw, in VSCode, I can add a new branch from the commit where I changed the aliases and then reset this PR to the commit before - should I do this? - but I don't think there's much benefit)

The only remaining mystery is the issue in the vignette:

Error in `map()`:
ℹ In index: 1.
ℹ With name: vignettes/selection_syntax.Rmd.
Caused by error:
! (converted from warning) No column names that matched the required search pattern were found.
Backtrace:
 1. my_function(iris, starts_with("Sep"))
 2. datawizard::extract_column_names(data, select = selection)
 3. insight::format_warning("No column names that matched the required search pattern were found.")
 4. insight::format_alert(..., type = "warning")

Quitting from lines 193-203 [unnamed-chunk-14] (selection_syntax.Rmd)

Not sure why this should suddenly fail.

@etiennebacher
Copy link
Member

I'm not sure it's worth the effort splitting this PR (I just saw, in VSCode, I can add a new branch from the commit where I changed the aliases and then reset this PR to the commit before - should I do this?

If you talk about #503 I think it also includes the changes for the group_by args so merging it first wouldn't change anything I think

The only remaining mystery is the issue in the vignette:

I can reproduce this warning locally with this branch but not with main so it probably comes from the alias removing.

I will try to split this PR this weekend if it's fine for you, or are you in a hurry for this one?

@strengejacke
Copy link
Member Author

No, no hurry.

@etiennebacher
Copy link
Member

@strengejacke I fixed the error in the vignette here: b8ac8ea. FYI those instructions were helpful to know how to move the latest N commits to a new branch (still took some manual tweaking though)

pkgdown workflows still fail but it's something different on which we don't have much control I think

@etiennebacher
Copy link
Member

We have three different test failures but all the rest is green (??):

  • ubuntu-latest 3.6: will go away when we bump version to >= 4.0 anyway
  • windows-latest 4.1: I think due to xfun but still super weird because only happens in this case
  • ubuntu-latest 4.3: function 'chm_factor_ldetL2' not provided by package 'Matrix', I don't think we can do much on that

=> I'm merging

@etiennebacher etiennebacher merged commit d9a40dc into main May 18, 2024
24 of 29 checks passed
@etiennebacher etiennebacher deleted the rename_group_by branch May 18, 2024 20:43
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.

None yet

2 participants