-
Notifications
You must be signed in to change notification settings - Fork 8
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
combine_checkboxes #196
Merged
Merged
combine_checkboxes #196
Changes from 5 commits
Commits
Show all changes
39 commits
Select commit
Hold shift + click to select a range
9b0471b
Reduce function initial draft
859926f
Reduce function fixes
c42399d
Small fixes
8862ac1
Draft tests, add no_val param
96c3309
Add keep param
218fca4
Fix `keep` param
54e3a99
Update documentation and API
ec5c19d
Update combine_checkbox api and docs
6b1fb48
Add check for if no fields exist in selection
d55dc00
Add check_fields_are_checkboxes function
261342d
Minor cleaning
e1d4eb8
Update version, test recheck workflow
7207f09
Test recheck workflow file
4f861e1
Fix linting
eb11152
Add combine_checkboxes() to pkgdown
7348324
Remove revdepcheck, update renv
62080af
Add standard checks for params
522d01d
Filename update
3a395cf
Filename change
347d2a3
Rename test file
rsh52 cce0d12
Fix record_id_field assign, remove rowwise call
rsh52 c250eda
Remove instrument_identifiers, use bind_cols
rsh52 b0a8564
Implement parse_labels, clean code, fix tests
rsh52 21f8879
Remove record_id field, lint
rsh52 31797c6
Apply additional cleanup suggestions
rsh52 2dfac9a
Add extract_metadata fnctn, tests
rsh52 ed55292
Support multiple values_to, logicals, new checks
c0b3885
Linting
7789a22
Update API, clean up, new methods, new docs
c185e39
Add check_metadata_fields_exist, update details
abdc512
Consoldiate and rework checkbox value conversion
50d47d6
Add names_repair strategy support
a6d150d
Remove names_suffix, restructure prefix/sep
0f868b8
Add names_glue spec
abefbee
Add glue support with names_glue
06d1337
Make glue dependency, remove install check
rsh52 dcb1029
Update glue spec handling
rsh52 0295650
check_equal_col_summaries() implementation
rsh52 127dd46
Update error message check_equal_col_summaries()
rsh52 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Oops, something went wrong.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Oops, something went wrong.
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Am I understanding correctly that these parameters control the names of the new columns and not how variable names (ex.
race___0
) are parsed into names and values?A couple comments:
name___value
format and not giving the user any control over how that's parsed._race
. The default should probably just producerace
:names_glue
would be super valuable if we can do it 😊There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They control the structure of the names, but the names themselves come from
.value
inget_metadata_spec()
, i.e. the field name prior to the___
checkbox changes.How about we just add a
check_metadata_fields_exist()
inget_metadata_spec()
similar to what I have in the parent function forcheck_fields_exist()
? If checkbox names are changed and they don't appear in the metadatafield_name
column, we can throw an error and suggestion. We should expect users don't manipulate the metadata tibbles, but we need the connection between the metadata tibble and the data tibble to remain intact. I think this supports our "if you change things, you need to take some responsibility for them" mindset.I'm open to changing it, but when I was thinking through outputs I was worried about clashing with other possibly existing column names. See this example, if we're ok with that being the default in the event of a clash then I can rework this.
Click me
ugh... Fine. I knew it was coming but figured I'll get the rest of this ironed out first.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay I'm fine with this. I would also add something to the documentation noting the pattern we're looking for.
I would resolve this with a warning or possibly error if the fields already exist.
pivot_longer()
for example errors and directs the user to thenames_repair
parameter to provide a repair strategy:That may be too sophisticated for us but we may actually be able to recreate that behavior pretty easily with
vctrs::vec_as_names()
which is referenced in the docs for thenames_repair
parameter.Haha if it ends up being too tricky that's fine!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright added support for
names_repair
andnames_glue
.names_glue
I'm still iffy on, the use case in thepivot_wider()
documentation is a bit more complicated than I believe we can support here, but try the current set up out and let me know what you think.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree that
pivot_wider()
supports more but I think we're still providing a lot value with what we have.Imagine a user has a
meals
instrument with some checkboxes like this:They could do:
How slick is that?