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

Add redcap_event_read() as wrapper for Export Events API method #460

Merged
merged 5 commits into from
Jan 3, 2023

Conversation

ezraporter
Copy link
Contributor

Closes #457

I adhered pretty closely to the template of redcap_arm_export() here. For a few of the tests I added data to existing directories in inst/test-data/. I went with the naming convention event.csv for the raw csv response from the Export Events methods but feel free to rename!

Copy link
Member

@wibeasley wibeasley left a comment

Choose a reason for hiding this comment

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

@ezraporter, this looks fantastic. Thanks for paying attention to all the little details and conventions of the package. I pointed out a few cosmetic requests.

Unrelated to your code, I'm surprised why the API returns arm_num, but the table's field is arm_id. I guess the PHP code is renaming it.

The only conceptual issue I'm considering is the data type of event_id. Admittedly this is fairly minor too. In the redcap_events_metadata table, the event_id field is an int(10). I think it would be easier if R uses and integer instead of a floating point. I think this needs only three changes.

  • Once in the real function's col_types definition, and
  • twice the test code's col_types.

Also,

  • please give yourself credit in the description file as a contributor

image

image

tests/testthat/test-redcap-event-read.R Outdated Show resolved Hide resolved
tests/testthat/test-redcap-event-read.R Outdated Show resolved Hide resolved
tests/testthat/test-redcap-event-read.R Outdated Show resolved Hide resolved
tests/testthat/test-redcap-event-read.R Outdated Show resolved Hide resolved
R/redcap-event-read.R Outdated Show resolved Hide resolved
R/redcap-event-read.R Show resolved Hide resolved
R/redcap-event-read.R Show resolved Hide resolved
R/redcap-event-read.R Show resolved Hide resolved
R/redcap-event-read.R Outdated Show resolved Hide resolved
tests/testthat/test-redcap-event-read.R Outdated Show resolved Hide resolved
@ezraporter
Copy link
Contributor Author

@wibeasley, thanks for looking at this! I think I addressed (and accepted) all your suggestions. Looks like I'm already in the DESCRIPTION from that bug fix a couple months ago so should be all set on that.

@ezraporter
Copy link
Contributor Author

@wibeasley happy new year! No rush on this but wanted to check where you were

@wibeasley wibeasley self-assigned this Jan 3, 2023
@wibeasley wibeasley merged commit eb7306c into OuhscBbmc:main Jan 3, 2023
@wibeasley
Copy link
Member

@ezraporter, the changes and everything look great. Thanks for doing it and sorry I missed the Dec 16 changes.

@ezraporter
Copy link
Contributor Author

No worries! Thanks!

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

Successfully merging this pull request may close these issues.

Add wrapper for Export Events API method to export event names
2 participants