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

use readr::read_csv() in checkbox_choices() #500

Closed
wibeasley opened this issue Jul 14, 2023 · 2 comments
Closed

use readr::read_csv() in checkbox_choices() #500

wibeasley opened this issue Jul 14, 2023 · 2 comments
Assignees

Comments

@wibeasley
Copy link
Member

@BlairCooper is threatening to try more irregular checkbox scenarios. The checkbox_choices() is using a single regex pattern to process the whole string, and it is getting pretty complicated for me (and I unironically love regexes):

pattern_checkboxes <- "(?<=\\A| \\| |\\| )(?<id>\\d{1,}), (?<label>[^|]{1,}?)(?= \\| |\\| |\\Z)"

readr's csv parser is widely tested against a lot of good & bad csvs styles, so let's use that instead.

I popped this little snippet into checkbox_choices() and all the tests passed immediately.

  strsplit(select_choices, split = "|", fixed = TRUE)[[1]] |>
    I() |>
    readr::read_csv(
      col_names = c("id", "label"),
      col_types = "cc"
    )

@BlairCooper, do you want to try this new branch with your scenarios?

Unless a pipe is used (for some other reason than separating choices) or a comma is used (for some other reason than separating the id from the label), I don't see how this can be improved to be more robust or maintainable.

@wibeasley wibeasley self-assigned this Jul 14, 2023
wibeasley added a commit that referenced this issue Jul 14, 2023
wibeasley added a commit that referenced this issue Jul 15, 2023
wibeasley added a commit that referenced this issue Jul 15, 2023
wibeasley added a commit that referenced this issue Jul 15, 2023
@BlairCooper
Copy link
Contributor

BlairCooper commented Jul 15, 2023

@wibeasley
One thing I like about the current implementation is that it exercises regex_named_captures() as well. I'm my next pull request I'll include some documentation on the RegEx.

I also tested your snippet and it does work though it is slower. Ran the metadata-utilities test three times each with regex and csv. regex ran pretty consistently at 0.3 seconds, csv ran pretty consistently at 0.5 seconds.

Side note: It is actually now not possible to create these scenarios in REDCap 13.7.5. Looks like they've added JavaScript to clean up the options when the field is saved. I was encountering these issues with fields that had been setup with an earlier version of REDCap. So you can't create these scenarios in 13.7.5 but they can still exist in that version.

@wibeasley
Copy link
Member Author

close issue. Replaced by #504

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

No branches or pull requests

2 participants