search

drisso / SingleCellExperiment

Clone of the Bioconductor repository for the SingleCellExperiment package, see https://bioconductor.org/packages/devel/bioc/html/SingleCellExperiment.html for the official development version.
63 stars 17 forks source link

Add tests for altExp error messages #38

Closed LTLA closed 4 years ago

LTLA commented 4 years ago

Pretty much as it says - cover the error messages.

LTLA commented 4 years ago

We had a deal, @kevinrue.

kevinrue commented 4 years ago

Damn. 10 days ago already. Didn't see that time go by, buddy. (I was travelling half of last week though..)

kevinrue commented 4 years ago

Tests pass on my side.

Can you be a bit clearer about what you mean here though? I can't afford the time for guesswork right now. Pointers to lines to fixes or at least one example for each type of fix would help a lot. Throw me a checklist that I can tick and we're back in business.

Also, here are my comments on the last couple of things you mentioned in the prematurely closed PR (https://github.com/drisso/SingleCellExperiment/pull/36):

altExps<- needs to enforce names in the same way that reducedDims<- does.

altExps <- does enforce names. Not sure what more you want.

Also, there are a bunch of tryCatch statements for combining and subsetting; these need to use conditionMessage() as well.

tryCatch statements in both subset.R and combine.R use conditionMessage(err) already. Again, not sure what you mean.

LTLA commented 4 years ago

altExps <- does enforce names. Not sure what more you want.

Yeah, cause I made it do so. Consider that comment obselete.

Again, not sure what you mean.

Again, because I just went ahead and did it.