refactor: updates of documentation and extension of unit tests #753
Conversation
jorainer
commented
Jun 6, 2024
jorainer
commented
- Small fixes and formatting changes in the code.
- Expand unit tests to cover additional scenarios.
- Update/restructure documentation.
- Small fixes and formatting changes in the code. - Expand unit tests to cover additional scenarios. - Update/restructure documentation.
| old <- paste0(old, "/") | ||
| if (length(spectraPath) > 0) { | ||
| old <- common_path(dataStorage(b)) | ||
| ## if (nchar(old) > 0) |
There was a problem hiding this comment.
not sure if we need this - I would skip since / and // work both, so better to be sure that there is a / before the file name.
There was a problem hiding this comment.
mmm I need to test it, because I thought the result of common_path never resulted with a / at the end of the path, so i thought that was necessary
There was a problem hiding this comment.
After checking, isn't that how the function behave ? and we definitely need to add this / ?
There was a problem hiding this comment.
how I understood it (can be wrong), but yes, the result from common_path() never ends in a "/". so, assume we have "/some/path/some/file.txt", then the common path would be "/some/path/some" and assuming the user defines spectraPath = "/new/path". We would like to replace to get "/new/path/file.txt". If we always add a "/"at theold, oldwould be in this case"/some/path/some/"and thesub()call will replace"/some/path/some/"with"/new/path"- and we get"/new/pathfile.txt". that's why I would not append the trailing "/"` to the pattern we want to replace.
Also, "/new/path/file.txt" and "/new/path//file.txt" both work - so, if the user would provide spectraPath = "/new/path/" (i.e. with a trailing "/") it would/should still work.
There was a problem hiding this comment.
ooooh of course ! No that make complete sense, but ye I think that the less we have if statement the better so i will remove it.
|
really big thanks for the great code and contribution @philouail ! |
| @@ -56,21 +94,38 @@ test_that("storeResults,loadResults,PlainTextParam,MsExperiment works", { | |||
| ## Loading data again | |||
| load_mse <- loadResults(object = MsExperiment(), param) | |||
| expect_true(inherits(load_mse, "MsExperiment")) | |||
| expect_equal(sampleData(mse), sampleData(load_mse)) | |||
| a <- spectra(mse) | |||
| expect_equal(sampleData(tmp), sampleData(load_mse)) | |||
There was a problem hiding this comment.
Can I ask why you create a tmp object here ? is it because otherwise we permanently change the mse (or xmse_full) object for the later tests ?
There was a problem hiding this comment.
I think testthat changes the variables within each unit test locally, not globally - it's more that when I evaluate the unit tests line by line I change the variables in my R session and then I get unexpected errors later. I generally avoid overwriting the "globally" defined variables where possible. Also, I specifically use a variable name tmp to remember that I will overwrite this variable for sure later...
|
Thanks so much @jorainer for polishing everything ! it looks so much better, I do have some questions in comments ! Also the checks don't pass because of the Spectra package update aha |
|
The updates in the Spectra package should become available later today or tomorrow I guess - I pushed yesterday to Bioconductor. |
