-
Notifications
You must be signed in to change notification settings - Fork 689
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
sageproteomics/sage #5677
sageproteomics/sage #5677
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
addressed via commit
Hey @dgemperline-lilly is this ready to be reviewed? If yes, I would update the branch, do a final pass and then approve if everything checks out! |
Yes good to go I think? |
@adamrtalbot I addressed the proposed changes via additional commit. Any way you can approve? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are just having a discussion about this in the maintainers team whether putting the file directly here is ok or whether you need to put it somewhere else. Does this file exist somewhere on github already?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes it does - originally had it linked on github publicly. We can revert if that is what is recommended.
@dgemperline-lilly I have added some review comments to fix remaining linting warnings. |
Co-authored-by: Florian Wuennemann <[email protected]>
Co-authored-by: Florian Wuennemann <[email protected]>
Co-authored-by: Florian Wuennemann <[email protected]>
Co-authored-by: Florian Wuennemann <[email protected]>
Co-authored-by: Florian Wuennemann <[email protected]>
@dgemperline-lilly The result from discussions between the maintainers team was pretty clear that test files cannot be in modules directly. Can you please make a PR to : https://github.com/nf-core/test-datasets/tree/modules with your test data? Feel free to ping me to review and approve! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Finally, please remove the file from this repo and move it to test-datasets. Sorry for the confusion!
@FloWuenne Opened pull request here: nf-core/test-datasets#1234 with the new file added. Will add in another commit once pull request goes through. |
@dgemperline-lilly Just approved the PR on test-datasets. Should be able to query that data in here now 😃 ! Thanks for adding the data to test-datasets David! |
Added key to test_dataset.config. Need approval here: |
OK last thing is to remove the old test file and make the two changes I suggested and we are good! 👍🏻 |
Removed old test file.
Co-authored-by: Florian Wuennemann <[email protected]>
Co-authored-by: Florian Wuennemann <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM now ⭐ !
PR checklist
Closes #XXX
versions.yml
file.label
nf-core modules test <MODULE> --profile docker
nf-core modules test <MODULE> --profile singularity
nf-core modules test <MODULE> --profile conda
nf-core subworkflows test <SUBWORKFLOW> --profile docker
nf-core subworkflows test <SUBWORKFLOW> --profile singularity
nf-core subworkflows test <SUBWORKFLOW> --profile conda