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 field type slider support #21

Merged
merged 12 commits into from
Feb 6, 2025

Conversation

ljwoodley
Copy link
Contributor

  1. I'm confused by the different metadata file here. Why not use one project/metadata file for all testing and development?

  2. The slider values here in select_choices_or_calculations field are above text_validation_max. Am I interpreting that correctly? If so, this makes tests/testthat/test-get_long_slider_field_values.R invalid.

@ljwoodley ljwoodley requested a review from pbchase February 2, 2025 14:43
Filter out BL is outside the scope of this function.
Add mention of slider support.
Reformat to satisfy the build checker.
Add shared_testdata folder.
Move best makedata.R script for metadata.csv to shared_testdata.
Revise tests to use shared_testdata/metadata.csv.
Regenerate metadata.csv to from REDCapR input.
Generate max_existing_id when test project is empty.
Generate unique values for each slider.
Assume the REDCap defaults for text_validation_min and text_validation_max if they are blank.
Localize metadata file used in test and add multiple slider fields.
Add test for unique values for each slider.
Don't separate on select_choices_or_calculations as the field is just labels.
Calculate the mean of min and max row-wise.
@pbchase
Copy link
Contributor

pbchase commented Feb 3, 2025

  1. I'm confused by the different metadata file here. Why not use one project/metadata file for all testing and development?

You have a valid point, so I made a shared metadata file and adjusted some tests to use it. Also, the metadata file I said was generated from the REDCap validation types file was modified. I updated the shared metadata file to be exactly what you get out of the REDCapR file.

All that said, I found flaws in this function revealed when I challenged it with multiple slider fields. So I couldn't use the shared, vanilla REDCapR metadata file unless I was willing to change the test data for every function. Depending on how I wrote the tests, that might break a lot of tests in one stroke. This is the object argument against shared test data. Each function might have specific needs that require specific test data.

I think there are two lessons here:

  1. Shared test data are advised until they fall short.
  2. Tests should be written to demonstrate features while ignoring the specifics of the test data.

@pbchase
Copy link
Contributor

pbchase commented Feb 3, 2025

2. The slider values here in select_choices_or_calculations field are above text_validation_max. Am I interpreting that correctly? If so, this makes tests/testthat/test-get_long_slider_field_values.R invalid.

The text in select_choices_or_calculations is a trio of labels for the slider. The values are in the min and max fields. I think you did the right thing.

@pbchase
Copy link
Contributor

pbchase commented Feb 3, 2025

@ljwoodley please review my changes

Copy link
Contributor

@pbchase pbchase left a comment

Choose a reason for hiding this comment

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

I made a lot of revisions and I'm happy with the result. Please check my work :-)

@ljwoodley ljwoodley merged commit 23a7bca into ctsit:develop Feb 6, 2025
2 checks passed
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.

2 participants