ropensci / BaseSet

Provides classes for working with sets
https://docs.ropensci.org/BaseSet
Other
10 stars 3 forks source link

rOpenSci review 1: empty sets? #38

Closed llrs closed 4 years ago

llrs commented 4 years ago

Document and test the handling of empty sets

A TidySet is a set of elements

* Empty sets:

  * `tidySet(list())`
  * `tidySet(list(a="A", b=character(0)))`
x <- list(a=c("A", "B"), b=character(0))
s <- tidySet(x)  # good - no errors are raised and the set looks right
elements(s)      # good 
sets(s)          # debatable - the empty set disappeared

The "right" behaviour of sets(s) is debatable, but whatever you decide, the behavior of edge cases should be documented and covered in the test suite. I have no problem with the empty set disappearing, though it could cause problems with downstream code if the user assumes every input set will be listed in sets(s).

In the sets documentation, I think the behavior would be clear if you just changed the Description to, "Given a TidySet, retrieve all non-empty sets or substitute them".

llrs commented 4 years ago

Empty sets are allowed but not shown, when printing. Will modify the documentation. The problem was with creating sets from list with empty characters. I enabled it but at the cost of not allowing to create an empty TidySet from a list. Not sure if it is worth to create an empty object to fill it later. I think there are enough methods to create a TidySet when needed (This also avoid the creation and extension pattern that is slow).

I didn't check for empty lists, now I added three more tests for these cases.