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

A few tweaks, typo corrections, and random cleanups #431

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
3 participants
@ryan-williams
Member

ryan-williams commented Oct 23, 2014

No grand plan here, just things I ran into and unconsciously fixed in the course of doing other things.

I've left them in separate commits for ease of review, lmk and I can squash+force-push.

@AmplabJenkins

This comment has been minimized.

Show comment
Hide comment
@AmplabJenkins

AmplabJenkins Oct 23, 2014

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

AmplabJenkins commented Oct 23, 2014

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

@fnothaft

This comment has been minimized.

Show comment
Hide comment
@fnothaft

fnothaft Oct 23, 2014

Member

Nice! This looks good. Let me just look more closely at 88ea343 first; I don't think it should be a no-op; rather, that function had utility a while ago...

Member

fnothaft commented Oct 23, 2014

Nice! This looks good. Let me just look more closely at 88ea343 first; I don't think it should be a no-op; rather, that function had utility a while ago...

@ryan-williams

This comment has been minimized.

Show comment
Hide comment
@ryan-williams

ryan-williams Oct 23, 2014

Member

hm yea you could be right, I may have been too hasty in my appraisal of adamRewriteContigIds; I'll have another look as well

Member

ryan-williams commented Oct 23, 2014

hm yea you could be right, I may have been too hasty in my appraisal of adamRewriteContigIds; I'll have another look as well

@ryan-williams

This comment has been minimized.

Show comment
Hide comment
@ryan-williams

ryan-williams Oct 23, 2014

Member

left a comment on 88ea343, I'm back in camp "down with adamRewriteContigIds!"

Member

ryan-williams commented Oct 23, 2014

left a comment on 88ea343, I'm back in camp "down with adamRewriteContigIds!"

@fnothaft

This comment has been minimized.

Show comment
Hide comment
@fnothaft

fnothaft Oct 24, 2014

Member

@ryan-williams I think you're indeed right about adamRewriteContigIds; let's go ahead with the change! Can you squash down? I'll merge when squashed.

Member

fnothaft commented Oct 24, 2014

@ryan-williams I think you're indeed right about adamRewriteContigIds; let's go ahead with the change! Can you squash down? I'll merge when squashed.

Several random cleanups:
- README tweak

- wrap some long lines

- clarify command parsing in ADAMMain

- quote bash var

- make `GenomicRegionPartitioner` a case class

  seems like it’s well-suited to be one:
  - public ctor vals
  - boilerplate .equals method
  - wrapper ctor that uses companion object method can go into companion
    object

- remove no-op `remapContig` function

  the “NB” comment about it being a no-op seems to be correct, afaict
@ryan-williams

This comment has been minimized.

Show comment
Hide comment
@ryan-williams

ryan-williams Oct 24, 2014

Member

cool, all squashed @fnothaft, thx

Member

ryan-williams commented Oct 24, 2014

cool, all squashed @fnothaft, thx

@fnothaft

This comment has been minimized.

Show comment
Hide comment
@fnothaft

fnothaft Oct 24, 2014

Member

Thanks @ryan-williams! I will merge once the tests finish.

Member

fnothaft commented Oct 24, 2014

Thanks @ryan-williams! I will merge once the tests finish.

@AmplabJenkins

This comment has been minimized.

Show comment
Hide comment
@AmplabJenkins

AmplabJenkins Oct 24, 2014

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

AmplabJenkins commented Oct 24, 2014

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

fnothaft added a commit that referenced this pull request Oct 24, 2014

Merge pull request #431 from ryan-williams/tweaks
[ADAM-431] Several random tweaks:
@fnothaft

This comment has been minimized.

Show comment
Hide comment
@fnothaft

fnothaft Oct 24, 2014

Member

Thanks @ryan-williams! Merged manually via 3df82e9.

Member

fnothaft commented Oct 24, 2014

Thanks @ryan-williams! Merged manually via 3df82e9.

@fnothaft fnothaft closed this Oct 24, 2014

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