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

New functionality for conditional validation errors #105

Merged
merged 10 commits into from
May 12, 2023
Merged

Conversation

elimillera
Copy link
Member

Per #78, Add argument to not error out if dataset validation fails.

Fairly simple, just adding a new argument and an if else statement

R/write.R Outdated Show resolved Hide resolved
Copy link
Collaborator

@bms63 bms63 left a comment

Choose a reason for hiding this comment

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

I'm a broken record today, but can we get a entry in News.md please! :)

@ddsjoberg
Copy link

Hello @elimillera ! Thank you for preparing this PR! Everything here looks perfect to me.

My only suggestion would be that we may not need an argument for this at all. I think to warn users it totally sufficient in this case. I don't know a situation where I need xportr to error due to sometimes minor non-adherence to all ADaM standards. But if someone does, they could always set options(warn=2) which treats warnings as errors.

Thanks again!

@elimillera
Copy link
Member Author

Thanks @ddsjoberg that's a really good point. I think sticking with the pattern of our other functions I would vote to keep the argument but I think changing the default to false is a better way to handle it. I think the arguments are useful if these is being used in a more automated pipeline where you want to stop execution if there are any issues like that in the output of the xpt file.

@elimillera elimillera requested a review from bms63 May 9, 2023 15:44
R/write.R Outdated Show resolved Hide resolved
R/write.R Outdated Show resolved Hide resolved
R/write.R Outdated Show resolved Hide resolved
tests/testthat/test-write.R Outdated Show resolved Hide resolved
Copy link
Collaborator

@bms63 bms63 left a comment

Choose a reason for hiding this comment

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

Just a couple wordsmithing comments

elimillera and others added 5 commits May 12, 2023 09:07
Co-authored-by: Ben Straub <ben.x.straub@gsk.com>
Co-authored-by: Ben Straub <ben.x.straub@gsk.com>
Co-authored-by: Ben Straub <ben.x.straub@gsk.com>
Co-authored-by: Ben Straub <ben.x.straub@gsk.com>
@elimillera elimillera requested a review from bms63 May 12, 2023 14:11
Copy link
Collaborator

@bms63 bms63 left a comment

Choose a reason for hiding this comment

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

LGTM!

@bms63 bms63 merged commit a0484ce into devel May 12, 2023
@bms63 bms63 deleted the 78-write-warning branch May 12, 2023 17:34
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.

Add option to print warning instead of error for validation failures in xportr::xportr_write()
3 participants