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

move DS validation from hard-coded to function driven #392

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

couthcommander
Copy link
Contributor

@couthcommander couthcommander commented Jun 27, 2024

This PR is to create a discussion. do not merge

Rather than hard-code validation into vectors/data.frames (which will not support branching logic with version), the code in "redcapDataStructure" could be updated into functions. This could grow into something very difficult to maintain. I have an incomplete version to at least start a dialogue about the best approach. I have moved the hard-coded information into a CSV file.

@couthcommander
Copy link
Contributor Author

Here's an example - the old approach:

REDCAP_SYSTEM_FIELDS <- c("redcap_event_name", 
                          "redcap_data_access_group", 
                          "redcap_repeat_instrument", 
                          "redcap_repeat_instance",
                          "redcap_survey_identifier")

incomplete approach:

REDCAP_SYSTEM_FIELDS <- .getStructure('f_SYSTEM', '14.4.0')

The final approach would replace all instances of "REDCAP_SYSTEM_FIELDS" with .getStructure('f_SYSTEM', rcon$version())

@spgarbet spgarbet marked this pull request as draft July 3, 2024 15:29
@spgarbet
Copy link
Member

spgarbet commented Jul 3, 2024

@couthcommander I changed this to a draft PR.

@spgarbet
Copy link
Member

@nutterb thoughts?

The existing structure is easier but will grow increasingly complex over time. This is to make it driven by an editable configuration file.

@nutterb
Copy link
Collaborator

nutterb commented Jul 12, 2024

Ignore my previous comment. For some reason I thought @couthcommander 's work used a json file somewhere. And then proceeded to describe exactly what he has coded.

I think I might be sleep deprived.

But yes, I think this is a necessary change. And having independently determined that this approach would be a feasible strategy, I'm on board with it.

@spgarbet
Copy link
Member

I'm ready for this to move from draft to being considered.

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.

3 participants