Various small fixes #907

huitseeker · 2015-12-29T21:11:39Z

some better option usage
removed some unneeded toStrings
some use of builtin functions for when applicable

disuse of builtin functions for common reduce operations unneceessary toString risky/anti-idiomatic Option usage

AmplabJenkins · 2015-12-29T21:13:00Z

Can one of the admins verify this patch?

fnothaft · 2015-12-29T21:14:11Z

Jenkins, test this please.

heuermh · 2015-12-29T21:18:37Z

adam-core/src/main/scala/org/bdgenomics/adam/converters/VariantAnnotationConverter.scala

@@ -113,7 +113,7 @@ object VariantAnnotationConverter extends Serializable {
 private def createFieldMap(keys: Seq[AttrKey], schema: Schema): Map[String, (Int, Object => Object)] = {
 keys.filter(_.attrConverter != null).map(field => {
 val avroField = schema.getField(field.adamKey)
- field.vcfKey -> (avroField.pos, field.attrConverter)
+ field.vcfKey -> ((avroField.pos, field.attrConverter))


sorry, what does this do?

The compiler is in this case auto-tupling and inserting the parentheses that I'm here making more explicit. See e.g. http://stackoverflow.com/a/29252334/47978

It's an ill-documented and now kind of deprecated feature.

thanks! yeah, that shouldn't have worked before :)

AmplabJenkins · 2015-12-29T21:34:55Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/ADAM-prb/1037/
Test PASSed.

heuermh · 2015-12-29T21:50:15Z

LGTM
@fnothaft merge this now or wait until #906 lands in case of conflicts?

fnothaft · 2016-01-05T10:06:11Z

Thanks for the great changes @huitseeker! I've dropped a variety of notes inline. Thanks for making a pass over the code and cleaning up a lot of detritus. It's especially good to see a lot of the .toString calls go away; these came in originally due to an issue in the way we were building the bdg-formats schemas, as they would return UTF8 strings instead of normal Java strings and would give all sorts of weird behavior.

- retire VcfStringUtils - finer exception types in some places - more explicit Fastq error message in stringent mode - allow IUPAC codes as isRegularBase alternatives

fnothaft · 2016-01-06T13:02:09Z

Thanks for the updated changes, @huitseeker! They look really good. I just had one last comment about the FASTQ exception; let me know what your thoughts are, and I think we can get this merged very soon.

huitseeker · 2016-01-06T14:38:53Z

Done ! Let me know what you think (and don't forget bigdatagenomics/utils#60 )

heuermh · 2016-01-06T17:01:50Z

Jenkins, retest this please

heuermh · 2016-01-06T17:01:57Z

LGTM, thanks!

AmplabJenkins · 2016-01-06T17:04:53Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/ADAM-prb/1040/

Build result: FAILURE

GitHub pull request #907 of commit aac5240 automatically merged.Notifying endpoint 'HTTP:https://webhooks.gitter.im/e/ac8bb6e9f53357bc8aa8'[EnvInject] - Loading node environment variables.Building remotely on amp-jenkins-worker-05 (centos spark-test) in workspace /home/jenkins/workspace/ADAM-prb > git rev-parse --is-inside-work-tree # timeout=10Fetching changes from the remote Git repository > git config remote.origin.url https://github.com/bigdatagenomics/adam.git # timeout=10Fetching upstream changes from https://github.com/bigdatagenomics/adam.git > git --version # timeout=10 > git fetch --tags --progress https://github.com/bigdatagenomics/adam.git +refs/pull/:refs/remotes/origin/pr/ > git rev-parse origin/pr/907/merge^{commit} # timeout=10 > git branch -a --contains ccdd5315a7094cab7a6af1c2c95754e6cf2c496e # timeout=10 > git rev-parse remotes/origin/pr/907/merge^{commit} # timeout=10Checking out Revision ccdd5315a7094cab7a6af1c2c95754e6cf2c496e (origin/pr/907/merge) > git config core.sparsecheckout # timeout=10 > git checkout -f ccdd5315a7094cab7a6af1c2c95754e6cf2c496eFirst time build. Skipping changelog.Triggering ADAM-prb ? 2.6.0,2.11,1.4.1,centosTriggering ADAM-prb ? 2.6.0,2.10,1.4.1,centosTouchstone configurations resulted in FAILURE, so aborting...Notifying endpoint 'HTTP:https://webhooks.gitter.im/e/ac8bb6e9f53357bc8aa8'
Test FAILed.

heuermh · 2016-01-06T17:08:27Z

Sorry, there's an error with the formatter

[INFO] --- scalariform-maven-plugin:0.1.4:format (default-cli) @ adam-parent_2.10 ---
[ERROR] Error formatting /home/jenkins/workspace/ADAM-prb/HADOOP_VERSION/2.6.0/SCALAVER/2.10/SPARK_VERSION/1.4.1/label/centos/adam-core/src/test/scala/org/bdgenomics/adam/rdd/read/MDTaggingSuite.scala: java.nio.charset.MalformedInputException: Input length = 1
[ERROR] Error formatting /home/jenkins/workspace/ADAM-prb/HADOOP_VERSION/2.6.0/SCALAVER/2.10/SPARK_VERSION/1.4.1/label/centos/adam-core/src/main/scala/org/bdgenomics/adam/rdd/read/AlignmentRecordRDDFunctions.scala: java.nio.charset.MalformedInputException: Input length = 1
[ERROR] Error formatting /home/jenkins/workspace/ADAM-prb/HADOOP_VERSION/2.6.0/SCALAVER/2.10/SPARK_VERSION/1.4.1/label/centos/adam-cli/src/main/scala/org/bdgenomics/adam/cli/PrintADAM.scala: java.nio.charset.MalformedInputException: Input length = 1
[INFO] Modified 1 of 199 .scala files

I'm for removing this from the build but will probably be outvoted.

heuermh · 2016-01-06T17:11:00Z

@fnothaft @massie somewhat related, how do we whitelist a contributor for Jenkins? follow up via email if necessary

huitseeker · 2016-01-06T17:56:18Z

@heuermh "add to whitelist"

Should be whitespace-only. Please view on github with '?w=1' appended to the url of this commit.

huitseeker · 2016-01-06T18:20:29Z

Updated to something more compliant. Let me know how it goes for you. I'm happy to revert the last commit if it goes against conventions

fnothaft · 2016-01-06T18:40:21Z

Thanks @huitseeker! This looks stellar from my side. @heuermh, this is good to go from your side as well, right? If so, I will squash this down later today and merge.

heuermh · 2016-01-06T19:07:00Z

Jenkins, add to whitelist

AmplabJenkins · 2016-01-06T19:15:28Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/ADAM-prb/1041/

Build result: FAILURE

GitHub pull request #907 of commit bb64ea6 automatically merged.Notifying endpoint 'HTTP:https://webhooks.gitter.im/e/ac8bb6e9f53357bc8aa8'[EnvInject] - Loading node environment variables.Building remotely on amp-jenkins-worker-05 (centos spark-test) in workspace /home/jenkins/workspace/ADAM-prb > git rev-parse --is-inside-work-tree # timeout=10Fetching changes from the remote Git repository > git config remote.origin.url https://github.com/bigdatagenomics/adam.git # timeout=10Fetching upstream changes from https://github.com/bigdatagenomics/adam.git > git --version # timeout=10 > git fetch --tags --progress https://github.com/bigdatagenomics/adam.git +refs/pull/:refs/remotes/origin/pr/ > git rev-parse origin/pr/907/merge^{commit} # timeout=10 > git branch -a --contains 144f41a # timeout=10 > git rev-parse remotes/origin/pr/907/merge^{commit} # timeout=10Checking out Revision 144f41a (origin/pr/907/merge) > git config core.sparsecheckout # timeout=10 > git checkout -f 144f41af508a898728a0c225aa8fbc7dd47d4c91First time build. Skipping changelog.Triggering ADAM-prb ? 2.6.0,2.11,1.4.1,centosTriggering ADAM-prb ? 2.6.0,2.10,1.4.1,centosTouchstone configurations resulted in FAILURE, so aborting...Notifying endpoint 'HTTP:https://webhooks.gitter.im/e/ac8bb6e9f53357bc8aa8'
Test FAILed.

huitseeker · 2016-01-06T19:57:22Z

Guys, I can't really do anything re: the three last formatting errors, see:

huitseeker➜huitseeker/tmp/adam(BDGLinting✗)» git diff origin/master -- adam-core/src/test/scala/org/bdgenomics/adam/rdd/read/AlignmentRecordRDDFunctionsSuite.scala
huitseeker➜huitseeker/tmp/adam(BDGLinting✗)» git diff origin/master -- adam-core/src/test/scala/org/bdgenomics/adam/rdd/read/MDTaggingSuite.scala
huitseeker➜huitseeker/tmp/adam(BDGLinting✗)» git diff origin/master -- adam-cli/src/main/scala/org/bdgenomics/adam/cli/PrintADAM.scala       [20:55:44]
huitseeker➜huitseeker/tmp/adam(BDGLinting✗)»

fnothaft · 2016-01-06T20:02:28Z

Guys, I can't really do anything re: the three last formatting errors, see:

Sure, I'll take a look into this.

Squashed #907 and ran format-sources

heuermh · 2016-01-07T15:43:41Z

Closing in favor of #908

fnothaft · 2016-01-07T17:34:10Z

Thanks again @huitseeker for the great change! I squashed it down and fixed the source formatting issue that was causing the build to fail as PR #908.

huitseeker added 2 commits December 29, 2015 22:05

Cleaning up TODOs in Vcfutils

a5c2f07

Fix various minor code issues:

5306dc4

disuse of builtin functions for common reduce operations unneceessary toString risky/anti-idiomatic Option usage

heuermh reviewed Dec 29, 2015
View reviewed changes

Enact reviewer's commments:

eaedcc0

- retire VcfStringUtils - finer exception types in some places - more explicit Fastq error message in stringent mode - allow IUPAC codes as isRegularBase alternatives

Add debug output for Fastq record

ba6b1ee

huitseeker force-pushed the BDGLinting branch from aac5240 to ba6b1ee Compare January 6, 2016 18:02

Scalariform-powered formatting.

bb64ea6

Should be whitespace-only. Please view on github with '?w=1' appended to the url of this commit.

fnothaft mentioned this pull request Jan 7, 2016

Squashed #907 and ran format-sources #908

Merged

heuermh added a commit that referenced this pull request Jan 7, 2016

Merge pull request #908 from fnothaft/huitseeker-BDGLinting

af778e0

Squashed #907 and ran format-sources

heuermh closed this Jan 7, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Various small fixes #907

Various small fixes #907

huitseeker commented Dec 29, 2015

AmplabJenkins commented Dec 29, 2015

fnothaft commented Dec 29, 2015

heuermh Dec 29, 2015

huitseeker Dec 29, 2015

heuermh Dec 29, 2015

AmplabJenkins commented Dec 29, 2015

heuermh commented Dec 29, 2015

fnothaft commented Jan 5, 2016

fnothaft commented Jan 6, 2016

huitseeker commented Jan 6, 2016

heuermh commented Jan 6, 2016

heuermh commented Jan 6, 2016

AmplabJenkins commented Jan 6, 2016

heuermh commented Jan 6, 2016

heuermh commented Jan 6, 2016

huitseeker commented Jan 6, 2016

huitseeker commented Jan 6, 2016

fnothaft commented Jan 6, 2016

heuermh commented Jan 6, 2016

AmplabJenkins commented Jan 6, 2016

huitseeker commented Jan 6, 2016

fnothaft commented Jan 6, 2016

heuermh commented Jan 7, 2016

fnothaft commented Jan 7, 2016

Various small fixes #907

Various small fixes #907

Conversation

huitseeker commented Dec 29, 2015

AmplabJenkins commented Dec 29, 2015

fnothaft commented Dec 29, 2015

heuermh Dec 29, 2015

Choose a reason for hiding this comment

huitseeker Dec 29, 2015

Choose a reason for hiding this comment

heuermh Dec 29, 2015

Choose a reason for hiding this comment

AmplabJenkins commented Dec 29, 2015

heuermh commented Dec 29, 2015

fnothaft commented Jan 5, 2016

fnothaft commented Jan 6, 2016

huitseeker commented Jan 6, 2016

heuermh commented Jan 6, 2016

heuermh commented Jan 6, 2016

AmplabJenkins commented Jan 6, 2016

Build result: FAILURE

heuermh commented Jan 6, 2016

heuermh commented Jan 6, 2016

huitseeker commented Jan 6, 2016

huitseeker commented Jan 6, 2016

fnothaft commented Jan 6, 2016

heuermh commented Jan 6, 2016

AmplabJenkins commented Jan 6, 2016

Build result: FAILURE

huitseeker commented Jan 6, 2016

fnothaft commented Jan 6, 2016

heuermh commented Jan 7, 2016

fnothaft commented Jan 7, 2016