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

Make multiqc_report optional when emailing #2273

Merged
merged 9 commits into from
May 8, 2023
Merged

Make multiqc_report optional when emailing #2273

merged 9 commits into from
May 8, 2023

Conversation

krokicki
Copy link
Contributor

@krokicki krokicki commented May 3, 2023

This PR addresses #2272 to make multiqc_report optional during the email step. This is useful for non-genomics pipelines that do not have multiqc support currently.

It looks likely that this fix will also address #1399 but I haven't tested that.

PR checklist

  • This comment contains a description of changes (with reason)
  • CHANGELOG.md is updated
  • If you've fixed a bug or added code that should be tested, add tests!
  • Documentation in docs is updated

@krokicki krokicki requested a review from ewels May 3, 2023 18:01
Copy link
Member

@ewels ewels left a comment

Choose a reason for hiding this comment

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

Nice, but I feel like you're changing more here than you need to. The try/catch block and default function arguments make me think that this was written to tolerate a missing MultiQC report already. Then at some point the type casting stuff was added and that broke.

I think that all we need to change is the params.containsKey('max_multiqc_email_size') condition and it should all work?

nf_core/pipeline-template/lib/NfcoreTemplate.groovy Outdated Show resolved Hide resolved
nf_core/pipeline-template/lib/NfcoreTemplate.groovy Outdated Show resolved Hide resolved
@@ -128,8 +130,12 @@ class NfcoreTemplate {
def email_html = html_template.toString()

// Render the sendmail template
def max_multiqc_email_size = params.max_multiqc_email_size as nextflow.util.MemoryUnit
def smail_fields = [ email: email_address, subject: subject, email_txt: email_txt, email_html: email_html, projectDir: "$projectDir", mqcFile: mqc_report, mqcMaxSize: max_multiqc_email_size.toBytes() ]
def max_multiqc_email_size = params.containsKey('max_multiqc_email_size') ? params.max_multiqc_email_size as nextflow.util.MemoryUnit : 0
Copy link
Member

Choose a reason for hiding this comment

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

Or just move this into the new if block and assume it exists?

Copy link
Member

Choose a reason for hiding this comment

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

Looking at #2272 I think that this line is basically the only one that needs to be fixed, the rest should probably already be tolerant to the report not being there (and look to me like they've been written with that in mind).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Very insightful! I've corrected the fix along these lines.

@codecov
Copy link

codecov bot commented May 5, 2023

Codecov Report

Merging #2273 (eae38cb) into dev (41b4dfc) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##              dev    #2273   +/-   ##
=======================================
  Coverage   73.12%   73.12%           
=======================================
  Files          77       77           
  Lines        8432     8432           
=======================================
  Hits         6166     6166           
  Misses       2266     2266           

Copy link
Member

@ewels ewels left a comment

Choose a reason for hiding this comment

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

Very tidy! 👏🏻 Thanks! 😄

@ewels ewels merged commit 8450f26 into nf-core:dev May 8, 2023
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.

3 participants