-
Notifications
You must be signed in to change notification settings - Fork 736
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
New attempt to add merfin/hist module #5352
Conversation
@itrujnara, any chances to review it again? Thanks for considering my request. |
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.
Nice! Stub test is missing, otherwise looks good.
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.
The new modules looks good now. I was puzzled by the random changes to other modules, but I assume these are for the stub test and thus relevant. Merge at your discretion.
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.
Thanks for updating the stubs
I'm glade to contribute! However I'm still getting an error related to genomescope2 running tests in conda environment. Any tips on why this is happening? |
I'm not entirely clear, but I'm going to make a PR to your branch at some point in the coming days to try and fix the test. |
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.
Requesting changes to remove the hack. The module itself should have been corrected first rather than add a hack to the module test. My apologies for not picking this up earlier.
See #5399 for the updated module PR for Genomescope2
#5401 should also remove the need to have the fixes for the linting checks here. |
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.
Aside from styling, there's a few things to clean up still and one thing to investigate.
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.
Nice work. Thank you. Sorry about all the changes.
No worries, thank you for your valuable tips and suggestions. I've learned a lot. |
This reverts commit 7805666e9d081dacb4c3cb5d0188bb8e9fc21773.
d341db5
to
fadc2e7
Compare
PR checklist
Closes #5136
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