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

Code style fixes #1299

Merged
merged 1 commit into from Dec 7, 2016

Conversation

Projects
None yet
5 participants
@heuermh
Member

heuermh commented Dec 2, 2016

Instead of finishing up docs on the plane last night, I ran through and made some code style fixes. Some may overlap with those in #1278.

@ryan-williams

This comment has been minimized.

Show comment
Hide comment
@ryan-williams

ryan-williams Dec 2, 2016

Member

nice! were you able to do these automatically somehow?

what's the advantage of finaling the things that you've done here?

Member

ryan-williams commented Dec 2, 2016

nice! were you able to do these automatically somehow?

what's the advantage of finaling the things that you've done here?

@heuermh

This comment has been minimized.

Show comment
Hide comment
@heuermh

heuermh Dec 2, 2016

Member

nice! were you able to do these automatically somehow?

Nope, a three hour flight with no kids helps. :)

what's the advantage of finaling the things that you've done here?

Good Java code style, see e.g. Effective Java "Item 17: Design and document for inheritance or else prohibit it" or the Checkstyle DesignForExtension check.

Member

heuermh commented Dec 2, 2016

nice! were you able to do these automatically somehow?

Nope, a three hour flight with no kids helps. :)

what's the advantage of finaling the things that you've done here?

Good Java code style, see e.g. Effective Java "Item 17: Design and document for inheritance or else prohibit it" or the Checkstyle DesignForExtension check.

@AmplabJenkins

This comment has been minimized.

Show comment
Hide comment
@AmplabJenkins

AmplabJenkins Dec 2, 2016

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

AmplabJenkins commented Dec 2, 2016

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

@fnothaft

This comment has been minimized.

Show comment
Hide comment
@fnothaft

fnothaft Dec 3, 2016

Member

what's the advantage of finaling the things that you've done here?

Good Java code style, see e.g. Effective Java "Item 17: Design and document for inheritance or else prohibit it" or the Checkstyle DesignForExtension check.

+1, akin to conservatively making methods private/protected.

Member

fnothaft commented Dec 3, 2016

what's the advantage of finaling the things that you've done here?

Good Java code style, see e.g. Effective Java "Item 17: Design and document for inheritance or else prohibit it" or the Checkstyle DesignForExtension check.

+1, akin to conservatively making methods private/protected.

@fnothaft

LGTM! I dropped a few nits inline that would be good (but not critical) to get before merging. Otherwise, looks great. Thanks for making the cleanup pass!

Show outdated Hide outdated ...-apis/src/main/scala/org/bdgenomics/adam/apis/java/JavaADAMContext.scala
object JavaADAMContext {
// convert to and from java/scala implementations
implicit def fromADAMContext(ac: ADAMContext): JavaADAMContext = new JavaADAMContext(ac)
implicit def toADAMContext(jac: JavaADAMContext): ADAMContext = jac.ac
}
class JavaADAMContext private (val ac: ADAMContext) extends Serializable {
class JavaADAMContext(val ac: ADAMContext) extends Serializable {

This comment has been minimized.

@fnothaft

fnothaft Dec 3, 2016

Member

While we're making this public, we should probably remove the implicits in the companion object, no? They'll be unusable from Javaland.

@fnothaft

fnothaft Dec 3, 2016

Member

While we're making this public, we should probably remove the implicits in the companion object, no? They'll be unusable from Javaland.

This comment has been minimized.

@heuermh

heuermh Dec 5, 2016

Member

I don't know the answer to this. It might be clearer to implement the change(s) in #1298 and then merge this one after that one.

@heuermh

heuermh Dec 5, 2016

Member

I don't know the answer to this. It might be clearer to implement the change(s) in #1298 and then merge this one after that one.

This comment has been minimized.

@fnothaft

fnothaft Dec 7, 2016

Member

I've just merged #1298. Want to rebase on that and reprise this point?

@fnothaft

fnothaft Dec 7, 2016

Member

I've just merged #1298. Want to rebase on that and reprise this point?

Show outdated Hide outdated adam-cli/src/test/scala/org/bdgenomics/adam/cli/AboutSuite.scala
@@ -48,6 +48,6 @@ class AboutSuite extends FunSuite {
assert(!about.scalaVersion().isEmpty)
assert(about.version() !== null)
assert(!about.version().isEmpty())
assert(!about.version().isEmpty)

This comment has been minimized.

@fnothaft

fnothaft Dec 3, 2016

Member

Might as well swap to nonEmpty here too.

@fnothaft

fnothaft Dec 3, 2016

Member

Might as well swap to nonEmpty here too.

Show outdated Hide outdated ...e/src/main/scala/org/bdgenomics/adam/converters/SAMRecordConverter.scala
@@ -107,7 +106,7 @@ private[adam] class SAMRecordConverter extends Serializable with Logging {
// set read alignment flag
val start: Int = samRecord.getAlignmentStart
assert(start != 0, "Start cannot equal 0 if contig is set.")
builder.setStart((start - 1))
builder.setStart((start - 1L))

This comment has been minimized.

@fnothaft

fnothaft Dec 3, 2016

Member

Remove extra parens here as well?

@fnothaft

fnothaft Dec 3, 2016

Member

Remove extra parens here as well?

@@ -149,8 +144,6 @@ private class FileFilter(private val name: String) extends PathFilter {
}
}
import org.bdgenomics.adam.rdd.ADAMContext._

This comment has been minimized.

@fnothaft

fnothaft Dec 3, 2016

Member

Nice! I was hoping we could get rid of this.

(On reflection, I don't know why I didn't just try deleting it in the first place!)

@fnothaft

fnothaft Dec 3, 2016

Member

Nice! I was hoping we could get rid of this.

(On reflection, I don't know why I didn't just try deleting it in the first place!)

@AmplabJenkins

This comment has been minimized.

Show comment
Hide comment
@AmplabJenkins

AmplabJenkins Dec 5, 2016

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

AmplabJenkins commented Dec 5, 2016

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

Show outdated Hide outdated adam-cli/src/main/scala/org/bdgenomics/adam/cli/Vcf2ADAM.scala
var variantContextsToSave = if (args.coalesce > 0) {
if (args.coalesce > variantContextRdd.rdd.partitions.size || args.forceShuffle) {
val variantContextsToSave = if (args.coalesce > 0) {
if (args.coalesce > variantContextRdd.rdd.partitions.length || args.forceShuffle) {

This comment has been minimized.

@devin-petersohn

devin-petersohn Dec 5, 2016

Member

We can cut this if then else out:
variantContextRdd.transform(_.coalesce(args.coalesce, shuffle = (args.coalesce > variantContextRdd.rdd.partitions.length || args.forceShuffle)))

@devin-petersohn

devin-petersohn Dec 5, 2016

Member

We can cut this if then else out:
variantContextRdd.transform(_.coalesce(args.coalesce, shuffle = (args.coalesce > variantContextRdd.rdd.partitions.length || args.forceShuffle)))

@heuermh

This comment has been minimized.

Show comment
Hide comment
@heuermh

heuermh Dec 6, 2016

Member

Addressed review comments. It might be more clear in git history to implement the change(s) in #1298 and then merge this pull request after that one.

Member

heuermh commented Dec 6, 2016

Addressed review comments. It might be more clear in git history to implement the change(s) in #1298 and then merge this pull request after that one.

@AmplabJenkins

This comment has been minimized.

Show comment
Hide comment
@AmplabJenkins

AmplabJenkins Dec 6, 2016

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

AmplabJenkins commented Dec 6, 2016

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

@devin-petersohn

Looks great to me.

@heuermh

This comment has been minimized.

Show comment
Hide comment
@heuermh

heuermh Dec 7, 2016

Member

Rebased and squashed commits after #1298 was merged.

Member

heuermh commented Dec 7, 2016

Rebased and squashed commits after #1298 was merged.

@AmplabJenkins

This comment has been minimized.

Show comment
Hide comment
@AmplabJenkins

AmplabJenkins Dec 7, 2016

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

AmplabJenkins commented Dec 7, 2016

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

@fnothaft fnothaft merged commit eb17701 into bigdatagenomics:master Dec 7, 2016

1 check passed

default Merged build finished.
Details
@fnothaft

This comment has been minimized.

Show comment
Hide comment
@fnothaft

fnothaft Dec 7, 2016

Member

Merged! Thanks @heuermh!

Member

fnothaft commented Dec 7, 2016

Merged! Thanks @heuermh!

@heuermh heuermh deleted the heuermh:style-fixes branch Dec 7, 2016

@heuermh

This comment has been minimized.

Show comment
Hide comment
@heuermh

heuermh Dec 7, 2016

Member

Thank you!

Member

heuermh commented Dec 7, 2016

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment