Skip to content

Closes #409 updated scorres to scstresc #410

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

Merged
merged 6 commits into from
Dec 14, 2024

Conversation

vbshuliar
Copy link
Collaborator

Summary:

This PR updates the checks check_sc_dm_eligcrit and check_sc_dm_seyeselc to replace all references of SCORRES with SCSTRESC as per the new requirements.

Changes:

Updated references of SCORRES to SCSTRESC in the following checks:

  • check_sc_dm_eligcrit
  • check_sc_dm_seyeselc

Testing:

Verified that the updated checks function correctly with SCSTRESC.

@vbshuliar vbshuliar added the good first issue Good for newcomers label Aug 22, 2024
@vbshuliar vbshuliar self-assigned this Aug 22, 2024
@barnett11
Copy link
Collaborator

Hi @vbshuliar - we should also update the tests within https://github.com/pharmaverse/sdtmchecks/tree/devel/tests/testthat

@barnett11
Copy link
Collaborator

Also may need to see what the source of the details for our checks for on this page - https://pharmaverse.github.io/sdtmchecks/articles/search_checks.html

@vbshuliar
Copy link
Collaborator Author

Hi reviewers,
I updated the tests as well. I'm not sure if everything looks how you expected, so I am more than happy to make changes according to your recommendations.

@barnett11
Copy link
Collaborator

Hi @vbshuliar - you may notice that there is a check failing for R CMD Check - this is identifying we've not updated a global variable for SCSTRESC. Please update R/globals.R for this

@vbshuliar
Copy link
Collaborator Author

Hi @vbshuliar - you may notice that there is a check failing for R CMD Check - this is identifying we've not updated a global variable for SCSTRESC. Please update R/globals.R for this

Hi @barnett11 !
I've just done it.

Copy link
Collaborator

@barnett11 barnett11 left a comment

Choose a reason for hiding this comment

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

Thanks @vbshuliar , some suggested updates thank you,

  • check_sc_dm_seyeselc description suggests OU should be an option, but actually it is not - please update description
  • Please also make same update of SCORRES to SCSTRESC for checks check_ec_sc_lat and check_oe_sc_lat_count_fingers

Thanks

@vbshuliar
Copy link
Collaborator Author

Hi @barnett11
Please check it now. I am happy to update anything else if needed.

@vbshuliar vbshuliar requested a review from barnett11 November 6, 2024 13:20
Copy link
Collaborator

@barnett11 barnett11 left a comment

Choose a reason for hiding this comment

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

All looking good thank you @vbshuliar !

@barnett11
Copy link
Collaborator

@harriscw - would you like to review before we merge to devel?

@harriscw harriscw merged commit 2ac0f17 into devel Dec 14, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants