Closes #164 Closes #228 Closes #234 xportr format messaging #243
Conversation
…Begin to add in tests for these checks.
…r these checks.
|
Due to the series of checks added to the |
|
I've run 
Anyone seen this before? It may just be a case that I need to delete my local git repo and re-start again. |
|
@bms63 I have added you as reviewer - but if there is someone else better placed to do this - please let me know. |
It looks like there's an error when it tries to install. What does |
|
tests/testthat/test-format.R
Outdated
There was a problem hiding this comment.
Could you create a few tests that show xportr_format() working with xport_type() and xportr_write().
unless you spot these tests are already being checked in other functions' test. Just want to make sure everything is playing nice together.
There was a problem hiding this comment.
Of course, will put these in place.
There was a problem hiding this comment.
Where would these tests be best placed, @bms63? If it's testing the interaction between varying functions - should a new script be created?
There was a problem hiding this comment.
I think it is fine to leave them in the test file for xportr_format(). My take on this is that we are focusing on testing the interactions of xportr_format with the other functions so it is a natural build up of tests...i.e., where we look at single use of the function and its arguments, possible bad input scenarios, writing out the xpt and then working well with others.
But it might be a good practice to document in our testing wiki on this testing strategy, e.g.
- Single focus tests on specific function and it arguments
- Possible data corruption scenarios and bad inputs
- Single focus tests Writing out xpt with specific function
- Tests showing interaction with other xportr functions
Any thoughts @atorus-research/xportr-development-team.
R/format.R
Outdated
| #' 1) If the variable has a suffix of `DT`, `DTM`, `TM` (indicating a | ||
| #' numeric date/time variable) then a warning will be shown if there is | ||
| #' no format associated with it. |
There was a problem hiding this comment.
The warning only fires when the user sets verbose = "warn" correct?
There was a problem hiding this comment.
No, that is not the case currently - will update so that this becomes the case.
There was a problem hiding this comment.
@bms63 - Would this also be the case for the 'message' also?
There was a problem hiding this comment.
NOTE TO SELF - need to update (as a result of this change...)
- Function documentation
- Tests to include
verboseoption - Additional test for a change in
verboseoption
There was a problem hiding this comment.
(Halfway through the tests - will continue tomorrow evening)
There was a problem hiding this comment.
@bms63 - Would this also be the case for the 'message' also?
If the users sets to 'nonethen nothing should come out of this function. The other functions are set up like this. It might be really good to make a test that usesxportr_metadata()andxportr()just to make sure these wrapper functions still work withxportr_format()` update
There was a problem hiding this comment.
There is already a test in test-xportr.R which tests that xportr() works - is this sufficient do you think, or would you like another? (It passes with the updates to xportr_format() still).
There was a problem hiding this comment.
if a similar test already exists then no worries.
maybe we should just have a test-xportr-fcns-interactions.R rather than my other proposed suggestion
There was a problem hiding this comment.
Really amazing work @sophie-gem !!! I was able to build the website. Not sure if you were able to solve your issues.
Many thanks for your detailed review @bms63 - really appreciate the feedback! Will start implementing now. :) |
Update NEWS.md according to changes requested. Co-authored-by: Ben Straub <ben.x.straub@gsk.com>
sophie-gem
changed the title
…r_format()` and added to the `xportr-utils.R` script.
You can boast the cyclomatic check up to 25 and this will pass. Thanks for making an issue around this!! |
| } | ||
|
|
||
| # if the variable is numeric | ||
| if (class(.df[[i]])[1] == "numeric") { |
There was a problem hiding this comment.
Is there any reason why you're comparing the class?
The same might be achieved using base R (this will return a single TRUE/FALSE)
| if (class(.df[[i]])[1] == "numeric") { | |
| if (is.numeric(.df[[i]])) { |
Or via the {checkmate} library
| if (class(.df[[i]])[1] == "numeric") { | |
| if (checkmate::test_numeric(.df[[i]])) { |
There was a problem hiding this comment.
I was using the class attribute as that is what is being used in the xportr_type() function - was trying to keep consistent.
|
I'm not entirely sure I understand? How would I go about doing this? |
|
@bms63 @averissimo , I'm getting slightly concerned that we are approaching the date that you'd like to release and am running out of capacity to get this completed...does anyone else have the bandwidth to help finish this off? The only things I believe are left to do are:
Other than that I believe all other changes requested have been made. Many thanks both for your detailed review - really appreciated! |
| @@ -1,5 +1,5 @@ | |||
| linters: linters_with_defaults( | |||
| cyclocomp_linter(complexity_limit = 18), | |||
| cyclocomp_linter(complexity_limit = 25), | |||
There was a problem hiding this comment.
Hey @sophie-gem this is where we can adjust complexity lintr. Just FYI most packages have a .lintr file to adjust things like this
Thank you for your Pull Request!
We have developed a Pull Request template to aid you and our reviewers. Completing the below tasks helps to ensure our reviewers can maximize their time on your code as well as making sure the xportr codebase remains robust and consistent.
The scope of
{xportr}{xportr}'s scope is to enable R users to write out submission compliantxptfiles that can be delivered to a Health Authority or to downstream validation software programs. We see labels, lengths, types, ordering and formats from a dataset specification object (SDTM and ADaM) as being our primary focus. We also see messaging and warnings to users around applying information from the specification file as a primary focus. Please make sure your Pull Request meets this scope of {xportr}. If your Pull Request moves beyond this scope, please get in touch with the{xportr}team on slack or create an issue to discuss.Please check off each task box as an acknowledgment that you completed the task. This checklist is part of the Github Action workflows and the Pull Request will not be merged into the
mainbranch until you have checked off each task.Changes Description
Following checks were added to the
formatfunction:DT,DTM,TMthen a warning is shown if no format associated.$prefix.$).$prefix.Relevant information added to the function documentation.
Tests for each of the checks above added to the function testthat file.
Task List
stylerpackage and functions to style files accordingly.devtools::document()so all.Rdfiles in themanfolder and theNAMESPACEfile in the project root are updated appropriatelypkgdown::build_site()and check that all affected examples are displayed correctly and that all new/updated functions occur on the "Reference" page.NEWS.mdif the changes pertain to a user-facing function (i.e. it has an@exporttag) or documentation aimed at users (rather than developers)NEWS.mdentry should go under the# xportr development versionsection. Don't worry about updating the version because it will be auto-updated using thevbump.yamlCI.