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

Export option exceptions #106

Merged
merged 18 commits into from
Oct 11, 2019
Merged

Export option exceptions #106

merged 18 commits into from
Oct 11, 2019

Conversation

PatrickRWright
Copy link
Collaborator

I added a new directory with test exports to do this. NEWS.md explains what has been fixed. Basically, if the information to add the pat_id, visit_name or centre columns is missing they are simply not added.

One review should do but I will invite you both.

closes #67
closes #103
closes #105

@codecov-io
Copy link

codecov-io commented Oct 4, 2019

Codecov Report

Merging #106 into master will decrease coverage by 0.25%.
The diff coverage is 84.84%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #106      +/-   ##
==========================================
- Coverage   93.67%   93.41%   -0.26%     
==========================================
  Files          20       20              
  Lines         996     1018      +22     
==========================================
+ Hits          933      951      +18     
- Misses         63       67       +4
Impacted Files Coverage Δ
R/labels_secuTrial.R 95% <0%> (ø) ⬆️
R/read_export_options.R 93.58% <100%> (+0.14%) ⬆️
R/read_export_table.R 97.43% <100%> (+0.06%) ⬆️
R/read_secuTrial_raw.R 94.54% <100%> (ø) ⬆️
R/read_secuTrial.R 81.81% <80.95%> (-18.19%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3d5ce81...64ec2ed. Read the comment docs.

@PatrickRWright
Copy link
Collaborator Author

I'm finished. Just thought of one more thing I wanted to test and added it while I was at it.

NEWS.md Outdated Show resolved Hide resolved
@aghaynes
Copy link
Member

aghaynes commented Oct 4, 2019

what does read_secutrial currently do? In my opinion, it would give warnings that the labels, etc are not applied, but still import the data. this could be done by checking if something is in the export options and then setting the 3 logicals to false if the setup stuff is not there...

@PatrickRWright
Copy link
Collaborator Author

Currently it will always try and add pat_id, centre and visit_name. This fails if the data for the procedure is missing.

I also considered adding warnings in this PR but decided not to.

I think that would be a work package related to #17. Once we have agreed on a set of suggested options (which will definitely include add-id, centre info and project setup) I suggest we tailor a verbose warning message for each option (if it differs from the suggestion) and point out what the effect is.

@aghaynes
Copy link
Member

aghaynes commented Oct 9, 2019

maybe it makes sense to skip factorize, dates and labels if project setup isn't available and give a warning (an if loop in read_secutrial would do i think)

@aghaynes aghaynes merged commit 460bc0e into SwissClinicalTrialOrganisation:master Oct 11, 2019
@PatrickRWright PatrickRWright removed the request for review from markomi October 11, 2019 14:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants