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.

Sign up for GitHub

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

Jump to bottom

[ADAM-1694] Add short readable descriptions for toString in subclasses of GenomicRDD. #1698

Merged
merged 1 commit into from
Sep 6, 2017
Merged

[ADAM-1694] Add short readable descriptions for toString in subclasses of GenomicRDD. #1698

merged 1 commit into from
Sep 6, 2017

Conversation

heuermh
Copy link
Member

@heuermh heuermh commented Aug 29, 2017

Fixes #1694

@AmplabJenkins
Copy link

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

@coveralls
Copy link

coveralls commented Aug 29, 2017 •
edited
Loading

Coverage Status

Coverage increased (+0.09%) to 83.519% when pulling 6ba1351 on heuermh:genomicrdd-tostring into 12c56fd on bigdatagenomics:master.

fnothaft
fnothaft requested changes Aug 30, 2017
View reviewed changes
Copy link
Member

@fnothaft fnothaft left a comment

Choose a reason for hiding this comment

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

General approach looks good, but can't we move this all into the GenomicRDD trait?

adam-core/src/main/scala/org/bdgenomics/adam/rdd/contig/NucleotideContigFragmentRDD.scala Outdated
@@ -490,4 +490,7 @@ sealed abstract class NucleotideContigFragmentRDD extends AvroGenomicRDD[Nucleot
(p._1, p._2: java.lang.Long)
}).toJavaRDD()
}

override def toString = "%s with %d reference sequences"
Copy link
Member

Choose a reason for hiding this comment

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

You should be able to factor this into the GenomicRDD trait, no?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think it is possible ... but I read some odd things about toString on traits and figured the sealed abstract classes would be the best place. Then the messages are slightly different so it isn't exactly breaking DRY.

Copy link
Member

Choose a reason for hiding this comment

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

What odd things did you see?

@heuermh heuermh modified the milestone: 0.23.0 Sep 1, 2017
@coveralls
Copy link

coveralls commented Sep 5, 2017 •
edited
Loading

Coverage Status

Coverage decreased (-0.1%) to 83.472% when pulling 5078c07 on heuermh:genomicrdd-tostring into 511f925 on bigdatagenomics:master.

@AmplabJenkins
Copy link

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

@fnothaft fnothaft merged commit 51efbaf into bigdatagenomics:master Sep 6, 2017
@fnothaft
Copy link
Member

fnothaft commented Sep 6, 2017

Merged! Thanks @heuermh!

@heuermh heuermh deleted the genomicrdd-tostring branch September 6, 2017 15:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fnothaft fnothaft fnothaft approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
0.23.0
Development

Successfully merging this pull request may close these issues.
4 participants
@heuermh @AmplabJenkins @coveralls @fnothaft