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

Explicitly check optional arguments if we are NOT loading FASTQ, and warn or throw. #1

Closed
wants to merge 9 commits into from

Conversation

fnothaft
Copy link

@fnothaft fnothaft commented Oct 2, 2015

LMK your thoughts here. I don't feel strongly one way or the other about adding this code in, but I thought it might be a useful check to have.

@ryan-williams
Copy link
Owner

I see what you mean now: a consequence of transform being the kitchen-sink that it is (which I am in favor of for now, fwiw; maybe "swiss-army-knife" is the better metaphor :) ) is that there are lots of possible subsets of the available cmdline flags that already don't make sense in the presence of each other.

I think that the cases you're guarding against here are worth checking but are also just a few of the cases that we maybe should check against (from a quick survey of all the available options to transform) if we want transform to have this contract with the user.

So, I'm inclined to think of this as work for another PR: "validate transform args to throw when a user passes inconsistent args".

Let me know if you think I'm mistaken about the existence of this problem predating bigdatagenomics#831 or if you think that the args I've introduced there are pushing us over the edge of needing to do this kind of validation, either just to the flags I've added or to all the flags to transform.

@fnothaft
Copy link
Author

fnothaft commented Oct 2, 2015

Agree with 100% of what you said! Let's push this to another PR; I've filed an issue: bigdatagenomics#841.

@fnothaft
Copy link
Author

fnothaft commented Oct 2, 2015

Closing in favor of bigdatagenomics#841.

@fnothaft fnothaft closed this Oct 2, 2015
ryan-williams pushed a commit that referenced this pull request Mar 28, 2016
tweak implicits in AlignmentRecordRDDFunctions
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.

2 participants