-
Notifications
You must be signed in to change notification settings - Fork 115
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
Dev #615
Dev #615
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.
Looks fine to me, but I know you discussed with @d4straub so maybe wait for another review from him.
Now I see some tests fail because of updates with 2.9 tools. @d4straub updated to the new template yesterday. |
Co-authored-by: Daniel Lundin <[email protected]>
Co-authored-by: Daniel Lundin <[email protected]>
Co-authored-by: Daniel Lundin <[email protected]>
Co-authored-by: Daniel Lundin <[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.
That looks thorough, great!
Could you merge the recently added PR #614? That should solve the failing test nf-core linting / nf-core
. nf-core linting / EditorConfig
is caused by trailing whitespaces, you could avoid that by using VSC and nf-core extensions, if you wished to (or just take care manually). nf-core linting / PythonBlack
can be solved by black.
…ow that happens in some test profiles
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.
Looks good to me. Do you want to merge it?
TAX <- tax_table(tax_mat) | ||
phy_obj <- phyloseq(OTU, TAX) | ||
|
||
if (file.exists($sam_tsv)) { |
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.
I am not aware of any example for conditional input that uses file.exists($file)
, so I am a little skeptic here. It might be working fine locally and on github, but we will need to see whether that works also on other systems. But easy enough to change in case it needs to be modified.
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.
That's a good point. I did my testing on a Linux server, but I'm hoping it would work on other systems.
Addresses #612
PR checklist
nf-core lint
).nextflow run . -profile test,docker --outdir <OUTDIR>
).docs/output.md
is updated.CHANGELOG.md
is updated.README.md
is updated (including new tool citations and authors/contributors).