Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

feat: accept NA in countElements; see #61 #62

Merged
merged 3 commits into from
Aug 4, 2023
Merged

Conversation

sgibb
Copy link
Member

@sgibb sgibb commented Aug 1, 2023

This PR allows NAs in subtractElements and addElements by handling NAs in countElements (problem described in #61 )

subtractElements("H2O", NA) returns "H2O". Or should it return NA?

Unfortunately it introduce a strange behaviour for containsElements:

containsElements("H2O", NA)
# [1] TRUE

IMHO containsElements("H2O", NA) should return FALSE or NA.

@jorainer, @michaelwitting any suggestion for the expected behaviour?

@jorainer
Copy link
Member

jorainer commented Aug 2, 2023

IMO the following would make sense:

containsElements("H2O", NA) should return NA
substractElement("H2O", NA) returns NA_character_
containsElements("H2O", "") should return FALSE (same as e.g. containsElements("H2O", "Z"))

with the current PR you're also reporting TRUE for containsElements("H2O", "Z")... thus, it always returns TRUE even if the provided element does not exist...

Copy link
Member

@jorainer jorainer left a comment

Choose a reason for hiding this comment

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

See comment - basically I would suggest to change the behavior to return NA for countElements("H2O", NA) as well as for subtractElement or addElement where a NA is provided.

@sgibb
Copy link
Member Author

sgibb commented Aug 2, 2023

containsElements("H2O", NA) should return NA substractElement("H2O", NA) returns NA_character_

done!

containsElements("H2O", "") should return FALSE (same as e.g. containsElements("H2O", "Z"))
with the current PR you're also reporting TRUE for containsElements("H2O", "Z")... thus, it always returns TRUE even if the provided element does not exist...

This wasn't introduced by this PR but is a bug that was/is already present. I add an issue #63 .

IMHO we could close #61 with this PR and fix #63 in another one.

Copy link
Member

@jorainer jorainer left a comment

Choose a reason for hiding this comment

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

Thanks @sgibb for the PR! Can you please also bump the version and add an entry to the NEWS file? apart from that all OK from me to merge.

@sgibb sgibb merged commit e95886f into main Aug 4, 2023
6 checks passed
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants