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

list_select Function Implementation #197

Closed
wants to merge 20 commits into from
Closed

list_select Function Implementation #197

wants to merge 20 commits into from

Conversation

rsh52
Copy link
Collaborator

@rsh52 rsh52 commented Jul 16, 2024

Description

This PR introduced a very low-level function that we've found useful in working with the CGTTrialsReporter. A typical workflow we found was to use extract_tibbles() on a REDCap supertibble and then select specific named elements from that list output for our analytic objects. This has the additional benefit

A typical workflow, as documented in the roxygen example, is:

my_list <- extract_tibbles(superheroes_supertbl)
list_select(my_list, starts_with("hero"))

Open to name and param suggestions, additional checks, behavior modifications, etc. I originally wanted to call this supertibble_select(), but this doesn't really select from the supertibble. If anything extract_tibbles() already achieves this.

Currently if users make a tidyselection and nothing is returned, an empty named list is returned. But if they hard code a table to search for and it doesn't exist and helpful error is returned. Wasn't sure if both should error instead.

Proposed Changes

List changes below in bullet format:

  • Introduce list_select() function
  • Add tests
  • Add to pkgdown

PR Checklist

Before submitting this PR, please check and verify below that the submission meets the below criteria:

  • New/revised functions have associated tests
  • [NA] New/revised functions that update downstream outputs have associated static testing files (.RDS) updated under inst/testdata/create_test_data.R
  • New/revised functions use appropriate naming conventions
  • New/revised functions don't repeat code
  • Code changes are less than 250 lines total
  • [NA] Issues linked to the PR using GitHub's list of keywords
  • The appropriate reviewer is assigned to the PR
  • The appropriate developers are assigned to the PR
  • Pre-release package version incremented using usethis::use_version()

Code Review

This section to be used by the reviewer and developers during Code Review after PR submission

Code Review Checklist

  • I checked that new files follow naming conventions and are in the right place
  • I checked that documentation is complete, clear, and without typos
  • I added/edited comments to explain "why" not "how"
  • I checked that all new variable and function names follow naming conventions
  • I checked that new tests have been written for key business logic and/or bugs that this PR fixes
  • I checked that new tests address important edge cases

@rsh52 rsh52 self-assigned this Jul 16, 2024
@rsh52 rsh52 added the enhancement New feature or request label Jul 16, 2024
@rsh52 rsh52 requested a review from ezraporter July 16, 2024 20:43
Copy link
Collaborator

@ezraporter ezraporter left a comment

Choose a reason for hiding this comment

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

Code is obviously fine! My comments are about the API.

Internally we've used a list_select-like function because we've tended to start with:

data_all <- extract_tibbles(supertbl)

Is that really what we want users to do? What's the point of this intermediate named list object when you can just do:

extract_tibbles(supertbl, starts_with("tbl"))

It feels to me like we should just be encouraging users to do operations on the supertibble.

@rsh52
Copy link
Collaborator Author

rsh52 commented Jul 18, 2024

Yea, fair enough. Personally fine with closing this if we don't think the value is really there, it didn't take much time to put together.

@ezraporter
Copy link
Collaborator

Yeah my opinion is that the value isn't there for this but there is value in a more comprehensive set of functions that allows you to select from and manipulate the supertibble

@rsh52
Copy link
Collaborator Author

rsh52 commented Jul 18, 2024

Say no more 🫡

@rsh52 rsh52 closed this Jul 18, 2024
@rsh52 rsh52 deleted the supertibble_select branch August 27, 2024 17:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants