Get rid of PB NA warning and improve NA handling overall.#11
Open
MilesMcBain wants to merge 1 commit intodavidcsterratt:masterfrom
Open
Get rid of PB NA warning and improve NA handling overall.#11MilesMcBain wants to merge 1 commit intodavidcsterratt:masterfrom
MilesMcBain wants to merge 1 commit intodavidcsterratt:masterfrom
Conversation
Owner
|
A long time has passed - apologies for the long delay replying and thank you for the PR. If you are still interested, I'd be happy to consider a pull request with tests. |
Contributor
|
I'll do that if Miles cannot |
Owner
|
I'd be happy for either of you to do this - I'm guessing that with the elapsed time, this project might not be relevant for Miles any more? |
Author
|
You have my blessing @mdsumner. I must admit to having completely dropped all context relating to this. |
Contributor
|
I actually think this is covered by #9, I think we just happened to hit on the same problem at the same time. Happy to follow up if there's more to do but I think our PRs give equivalent behaviour. I'll try to look at testing at some point. |
Contributor
|
ah, I see that this PR adds more checks - having a closer look |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
I was going to make a fix just for #10 but then I noticed some issue with the way
NAis handled. IfNAaccidentally makes its way into a parameter, the whole thing will be wiped and replaced with a default. I think this is unexpected and it would be kinder to the user to stop and notify as withP.I made some explicit checks for a length 1
NAinstead ofany(is.na(param)), which is more precisely the case you wish to handle.If you are prepared to accept this pull, don't merge immediately, let me know and I'll add a few tests. :)