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

Check "CHART_OUTPUT != null" before generating a pdf #1850

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

yatzek
Copy link

@yatzek yatzek commented Jan 11, 2023

Description

Check if "CHART_OUTPUT" command line parameter was provided before generating a pdf report.

This is to match the logic in the "customCommandLineValidation" method which checks if R is installed
when "CHART_OUTPUT != null"
This can be helpful when running on environments that do not have or cannot have R installed.
You should be able to use this tool on such environments provided you do not ask for a pdf report.


Checklist (never delete this)

Never delete this, it is our record that procedure was followed. If you find that for whatever reason one of the checklist points doesn't apply to your PR, you can leave it unchecked but please add an explanation below.

Content

  • Added or modified tests to cover changes and any new functionality
  • Edited the README / documentation (if applicable)
  • All tests passing on Travis

Review

  • Final thumbs-up from reviewer
  • Rebase, squash and reword as applicable

For more detailed guidelines, see https://github.com/broadinstitute/picard/wiki/Guidelines-for-pull-requests

@lbergelson
Copy link
Member

@yatzek I fully support this plan. R is a constant irritation and we shouldn't require it when it's not absolutely necessary. I'm not sure this accomplishes the goal though. There are additional checks in some of those classes to make sure that the chart output file is writeable which will crash if you pass null through even with your change.

It's also a bit weird to have to pass null in an argument at all, maybe making CHART optional would make more sense? I guess people will forget it then though when they meant to include it.

@kachulis
Copy link
Contributor

@yatzek will you be able to respond to @lbergelson's comments?

@droazen
Copy link
Contributor

droazen commented Apr 11, 2023

@yatzek It seems that there are a couple of outstanding issues that need to be resolved before we could accept this PR:

  • CHART_OUTPUT should be made an optional argument
  • There are a couple additional usages of CHART_OUTPUT not caught by this PR that need null checks in order for this to work
  • There needs to be a test that demonstrates that the metrics collection can run successfully when CHART_OUTPUT is not specified.

Please let us know if you're able to make these changes

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.

None yet

4 participants