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

229 delayed_choices as separate funcitons #232

Merged

Conversation

chlebowa
Copy link
Contributor

Closes #229
Alternative to #231

CHANGES

Added first_choice and last_choice to extend all_choices functionality to subsets of available choices.

IMPLEMENTATION

all_choices, first_choice, and last_choice are functions of class delayed_choices. delayed_choices functions will add the appropriate subsetting operation to the $subset element of their delayed_data argument. Atomic arguments are returned as is.

if (inherits(selected) "delayed_choices") {
  selected <- selected(choices)              # modify delayed choices
} else {
  selected <- choices                        # use choices as is
}

all_choices is still a special case when checking arguments in some functions (choices vs multiple) so an extra class is added to the all_choices function to keep track of that.

@chlebowa chlebowa changed the title 229 delayed choices separate@main 229 delayed choices as separate funcitons Dec 19, 2024
@chlebowa chlebowa changed the title 229 delayed choices as separate funcitons 229 delayed_choices as separate funcitons Dec 19, 2024
@llrs-roche
Copy link

I like this approach more as all_choices is already released and this approach would keep it around.

About the exact implementation and other details I haven't reviewed it, but at first glance I'd like the explore the call to Reduce on R/delayed_choices.R lines 80 and 59. Maybe it can be reviewed & merged on the early January. Thanks!

@gogonzo gogonzo self-assigned this Jan 9, 2025
Copy link
Contributor

@gogonzo gogonzo left a comment

Choose a reason for hiding this comment

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

All good except commented

R/delayed_choices.R Outdated Show resolved Hide resolved
tests/testthat/test-delayed_choices.R Outdated Show resolved Hide resolved
R/delayed_choices.R Outdated Show resolved Hide resolved
chlebowa and others added 5 commits January 9, 2025 12:51
Co-authored-by: Dawid Kałędkowski <dawid.kaledkowski@gmail.com>
Signed-off-by: Aleksander Chlebowski <114988527+chlebowa@users.noreply.github.com>
@chlebowa
Copy link
Contributor Author

chlebowa commented Jan 9, 2025

@gogonzo Please have a look at my proposed compromise for the modification of x$subset.

@chlebowa chlebowa requested a review from gogonzo January 9, 2025 12:40
@chlebowa
Copy link
Contributor Author

chlebowa commented Jan 9, 2025

I have read the CLA Document and I hereby sign the CLA

@gogonzo gogonzo merged commit 0f647e6 into insightsengineering:main Jan 9, 2025
27 of 28 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Jan 9, 2025
@chlebowa chlebowa deleted the 229_delayed_choices_separate@main branch January 9, 2025 13:19
Comment on lines +81 to +82
added_fun(original_fun(x))
x
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
added_fun(original_fun(x))
x
added_fun(original_fun(data))

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature Request]: add first_choice function
3 participants