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

Getting rid of aliases for selection functions #265

Closed
IndrajeetPatil opened this issue Sep 14, 2022 · 8 comments · Fixed by #504
Closed

Getting rid of aliases for selection functions #265

IndrajeetPatil opened this issue Sep 14, 2022 · 8 comments · Fixed by #504
Labels
Breaking 🏴‍☠️ www.fcstpauli.com Upkeep 🧹 maintenance

Comments

@IndrajeetPatil
Copy link
Member

IndrajeetPatil commented Sep 14, 2022

Screenshot 2022-09-14 at 20 50 05

  • get_columns() is confusing because, unless you read the docs, it is not clear whether the function returns column names, or a list of vectors, or a data frame. data_select() is much better, since we have established that data_*() functions always return a data frame. Keeping the get_columns() around for long is inviting confusion.

  • The same is true for data_find(). All other data_*() functions return data frames, while this one returns an atomic vector. This messes user's mental model. We should remove it in favour of find_columns().

library(datawizard)
data_find(iris, starts_with("Sepal"))
#> [1] "Sepal.Length" "Sepal.Width"

Created on 2022-09-14 with reprex v2.0.2

This is probably something we can do in 0.7 or later.

@IndrajeetPatil IndrajeetPatil added Breaking 🏴‍☠️ www.fcstpauli.com Upkeep 🧹 maintenance labels Sep 14, 2022
@strengejacke
Copy link
Member

I thought the rational was that all data_*() functions only work with data frames (not what they return). Returning a data frame also works for center() et al, so we focus on the input when choosing the function name. But of course, we could say that data_*() means input and output are data frames, and thus remove data_find() in favor of find_columns(). find_columns() sounds more natural, and that's why we added get_columns(), in the tradition of insight's get_*/find_* pairs.

@IndrajeetPatil
Copy link
Member Author

and thus remove data_find() in favor of find_columns()

Okay, that sounds reasonable to me.

find_columns() sounds more natural

Maybe, but it's also more confusing. find_column_names() or extract_column_names() might have been better.
But I can make my peace with the current naming schema.

But I still think get_columns() should be removed in favour of data_select().

The fewer functions we have that the user needs to learn, the lower the cognitive load we are imposing on them.

Users are also likely to find it more familiar to work with.

Screenshot 2022-09-15 at 08 22 50

@DominiqueMakowski
Copy link
Member

find_/get_columns was indeed related to insight's logic, but I also agree that moving forward we should actually start deprecating and pruning aliases and focus on "the most easystatsy way" of doing things

@strengejacke
Copy link
Member

Maybe, but it's also more confusing. find_column_names() or extract_column_names() might have been better.
But I can make my peace with the current naming schema.

Yeah, I like find_column_names() or extract_column_names(). So we would deprecate both find_columns() and data_find(), and use one of the mentioned suggestions?

@strengejacke
Copy link
Member

find_/get_columns was indeed related to insight's logic, but I also agree that moving forward we should actually start deprecating and pruning aliases and focus on "the most easystatsy way" of doing things

agree.

@strengejacke
Copy link
Member

Screenshot 2022-09-15 at 08 22 50

btw, data_filter() both works like dplyr::filter() and dplyr::slice().

@IndrajeetPatil

This comment was marked as outdated.

@etiennebacher

This comment was marked as off-topic.

strengejacke added a commit that referenced this issue May 17, 2024
etiennebacher pushed a commit that referenced this issue May 18, 2024
etiennebacher added a commit that referenced this issue May 18, 2024
* easystats/easystats#404

* update snapshots

* lintr, comments

* fix demean()

* fix means_by_group

* fix

* update news

* fix rescale_weights

* silence tests

* Update NEWS.md

Co-authored-by: Etienne Bacher <[email protected]>

* deprecation warnings

* use insight remotes

* Update NEWS.md

Co-authored-by: Etienne Bacher <[email protected]>

* Also address #265

* update docs

* update docs and tests

* Update extract_column_names.R

* update readme

* trigger CI

* revert commits related to aliases

* version

* news

* other remnants

* fix

* do not use devel pkgdown

* lintr

* same

---------

Co-authored-by: Etienne Bacher <[email protected]>
etiennebacher added a commit that referenced this issue May 19, 2024
* Also address #265

* update docs

* update docs and tests

* Update extract_column_names.R

* update readme

* trigger CI

* fix

* do not remove find_columns yet

* fix news

* lintr, styler

* fix

* lintr

* add favicons to work around pkgdown bug

---------

Co-authored-by: Daniel <[email protected]>
Co-authored-by: Indrajeet Patil <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Breaking 🏴‍☠️ www.fcstpauli.com Upkeep 🧹 maintenance
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants