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

Move to bdg-formats #280

Merged
merged 1 commit into from
Jul 2, 2014
Merged

Move to bdg-formats #280

merged 1 commit into from
Jul 2, 2014

Conversation

fnothaft
Copy link
Member

Deletes the adam-format module, and moves the project to build off of the bdg-formats project.

fnothaft
fnothaft reviewed Jun 25, 2014
View reviewed changes
pom.xml
@@ -284,6 +283,11 @@
<dependencyManagement>
<dependencies>
<dependency>
<groupId>org.bdgenomics.bdg-formats</groupId>
<artifactId>bdg-formats</artifactId>
<version>0.1.1-SNAPSHOT</version>
Copy link
Member Author

Choose a reason for hiding this comment

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

Question: how do we step the formats/adam versions? Do we build off of a snapshot release of bdg-formats when ADAM is on a snapshot, and then build off of a release whenever we do a release?

Paging @hammer @heuermh @tdanford @carlyeks.

Copy link
Contributor

Choose a reason for hiding this comment

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

I was assuming that we'd never depend on a snapshot release.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe snapshots make sense as development; definitely if I am currently modifying bdg-formats, and testing changes to ADAM, I want to use the latest.

Copy link
Member Author

Choose a reason for hiding this comment

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

@massie and I went back and forth on this. We started with the same assumption as @tdanford and then figured that @carlyeks' point was also correct. If we did depend on a snapshot, we'd need to move to a release copy when we did the release.

@AmplabJenkins
Copy link

All automated tests passed.
Refer to this link for build results: https://amplab.cs.berkeley.edu/jenkins/job/ADAM-prb/49/

carlyeks
carlyeks reviewed Jun 26, 2014
View reviewed changes
adam-cli/src/main/scala/org/bdgenomics/adam/cli/ADAM2Vcf.scala
@@ -17,7 +17,7 @@
*/
package org.bdgenomics.adam.cli

import org.bdgenomics.adam.avro.ADAMGenotype
import org.bdgenomics.bdg_formats.avro.ADAMGenotype
Copy link
Member

Choose a reason for hiding this comment

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

I would rather just have "formats" as the namespace, "bdg_formats" is...underscore-y...

Copy link
Member

Choose a reason for hiding this comment

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

I also don't like bdg_formats in the package name very much.

I thought perhaps I'd look elsewhere for guidance, and e.g. ga4gh calls its project schemas, then uses ga4gh-format as its artifactId, and just org.ga4gh for the package name. So not much help there. :)

What about org.bdgenomics.avro for the package name and bgd-formats or bgd-schemas for the artifactId and project name?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's not my preference either, however, it seems that Java's style guide suggests changing a hypen into an underscore. See 1 and 2.

Copy link
Member

Choose a reason for hiding this comment

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

I'm with @carlyeks . I like 'formats' over 'bdg_formats' and submitted a pull request to change it (before I saw this comment thread).

Copy link
Member Author

Choose a reason for hiding this comment

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

@massie I've just merged bigdatagenomics/bdg-formats#7 and will update this PR as soon as the new snapshot is pushed.

@fnothaft
Copy link
Member Author

I've updated this after @massie's PR to change the bdg-formats path from bdg_formats to formats. Additionally, I've made a few changes to clean up build warnings.

@AmplabJenkins
Copy link

All automated tests passed.
Refer to this link for build results: https://amplab.cs.berkeley.edu/jenkins/job/ADAM-prb/54/

@fnothaft
Copy link
Member Author

fnothaft commented Jul 2, 2014

Ping...

@tdanford
Copy link
Contributor

tdanford commented Jul 2, 2014

Carl and I are checking this out right now -- but can you rebase off of master?

@fnothaft
Copy link
Member Author

fnothaft commented Jul 2, 2014

Consider it done @tdanford!

@tdanford
Copy link
Contributor

tdanford commented Jul 2, 2014

@fnothaft how would you feel, about splitting out the two sets of changes here into two PRs?

  1. all the structural changes relevant to the switch to bdg-formats (the pom update, the imports, deleting the adam-formats avdl) and
  2. the for loop conversions into map/foreachs, and the conversion of field references into accessor invocations?

@AmplabJenkins
Copy link

All automated tests passed.
Refer to this link for build results: https://amplab.cs.berkeley.edu/jenkins/job/ADAM-prb/58/

@fnothaft
Copy link
Member Author

fnothaft commented Jul 2, 2014

I've just split out the cleanup; I'll rebase that and open a new PR once this merges.

@AmplabJenkins
Copy link

All automated tests passed.
Refer to this link for build results: https://amplab.cs.berkeley.edu/jenkins/job/ADAM-prb/59/

carlyeks added a commit that referenced this pull request Jul 2, 2014
@carlyeks
Merge pull request #280 from fnothaft/move-to-bdgformats
c68cea9 
Move to bdg-formats
@carlyeks carlyeks merged commit c68cea9 into bigdatagenomics:master Jul 2, 2014
@carlyeks
Copy link
Member

carlyeks commented Jul 2, 2014

Glad to see we've finally moved over. Thanks, @fnothaft!

@fnothaft fnothaft mentioned this pull request Jul 5, 2014
Closed
@fnothaft fnothaft deleted the move-to-bdgformats branch July 10, 2014 13:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@fnothaft @AmplabJenkins @tdanford @carlyeks @massie @heuermh