Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

feat: add MsBackendMetaboLights import/export #20

Merged
merged 2 commits into from
Oct 22, 2024
Merged

feat: add MsBackendMetaboLights import/export #20

merged 2 commits into from
Oct 22, 2024

Conversation

jorainer
Copy link
Member

  • Add save/read support for MsBackendMetaboLights objects.

- Add save/read support for `MsBackendMetaboLights` objects.
@jorainer jorainer marked this pull request as draft October 17, 2024 13:36
@jorainer
Copy link
Member Author

Need to wait for the MsBackendMetaboLights updates.

@jorainer jorainer marked this pull request as ready for review October 21, 2024 12:53
dir.create(path = path, recursive = TRUE, showWarnings = FALSE)
saveObjectFile(path, "ms_backend_mz_r",
list(ms_backend_mz_r =list(version = "1.0")))
l <- list(list(version = version))
Copy link
Collaborator

Choose a reason for hiding this comment

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

I am a bit confused about the list(list()). and then you name it 'ms_backend_mzr` into it next line ? why does it need to be a list of a list ?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is the particular format saveObjectFile want the input parameter, i.e. a list with all optional parameters (one of them being version and the name of that list should be the name/type of the object. It's confusing, I agree, but I don't know how to call it differently, given that we want to support setting the type with parameter object of the upstream function.

@@ -48,6 +51,11 @@
#' `readMsObject()` functions for other classes (such as `Spectra`,
#' `MsExperiment` etc).
#'
#' @param offline For `readMsObject()` to load MS data as a
#' `MsBackendMetaboLights()`: `logical(1)` to evaluate the local file cache
#' and only load local files. Thus `offline = TRUE` does not need an active
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would avoid repetition maybe and instead maybe ..to evaluate **only** the local file cache ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point, thanks!

Copy link
Member Author

Choose a reason for hiding this comment

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

Done with the latest commit.

Copy link
Collaborator

@philouail philouail left a comment

Choose a reason for hiding this comment

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

I have small comments but otherwise everything is good for me ! thanks a lot

@jorainer
Copy link
Member Author

Thanks for the review Phili!

@jorainer jorainer merged commit bdbda4d into main Oct 22, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants