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: creation of new generic function so save XcmsExperiment object #698

Merged
merged 8 commits into from
Nov 7, 2023

Conversation

philouail
Copy link
Collaborator

Hi so this is just a draft, but I thought it might be easier to discuss here. I might need a bit of back and forth as it's the first time I do that :) Hope it's fine for you to do it like that. I'll also leave it as draft to avoid confusion.

See my comments for the parts that I'm confused about !

R/RDataParam.R Outdated Show resolved Hide resolved
@philouail
Copy link
Collaborator Author

philouail commented Oct 26, 2023

Oh and also I tried to do the necessary changes in description and NEWS files, but can you check if it looks OK ? Maybe there's different rules for XCMS compared to the other packages I contributed.
And I have to look into this NAMESPACE thing, to see how to do it manually tomorrow.

R/AllGenerics.R Outdated Show resolved Hide resolved
R/AllGenerics.R Outdated Show resolved Hide resolved
@jorainer
Copy link
Collaborator

For the NAMESPACE, just add

export RDataParam
exportMethods storeResults

that should be enough. Description and NEWS are OK - yes, there is a different format here, but what you added is fine.

@philouail philouail marked this pull request as ready for review October 31, 2023 13:19
@philouail
Copy link
Collaborator Author

Hi @sneumann so this PR is related to #693, it establish a new generics function storeResults that will allow to save xcms results objects into different format. This PR also implement the first format which is .RData with the RDataParam.
I will then look into the Plain text file format and lastly MzTab-m they will each have they own param class and specific method.

Tell me if there is anything unclear or I need to change something.

Copy link
Collaborator

@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 Phili! I'm fine with this PR.

Please @sneumann also have a look whether you're OK with the general approach we take here.

Only thing, @philouail , please address the one point regarding the dll file.

src/xcms.dll Outdated Show resolved Hide resolved
Copy link
Owner

@sneumann sneumann left a comment

Choose a reason for hiding this comment

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

Hi, looks good to me, thanks for the nice work !
Yours, Steffen

tests/testthat/test_XcmsExperiment.R Outdated Show resolved Hide resolved
@sneumann
Copy link
Owner

sneumann commented Nov 7, 2023

Cool, so ready to merge from my side.

@sneumann sneumann merged commit 522a6d7 into devel Nov 7, 2023
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants