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.

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

Fastq record converter #1208

Closed
wants to merge 2 commits into from
Closed

Fastq record converter #1208

wants to merge 2 commits into from

Conversation

fnothaft
Copy link
Member

Moving @zyxue's work over from #1185, but squashed down and with @heuermh's review comments in.

@fnothaft
Copy link
Member Author

fnothaft commented Oct 13, 2016

I will squash and merge on tests passing.

@fnothaft fnothaft mentioned this pull request Oct 13, 2016
@zyxue
Copy link
Contributor

zyxue commented Oct 13, 2016

@heuermh has added some more requests on #1185, I haven't got a chance to look into it yet. Will do later.

@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/1534/
Test PASSed.

@fnothaft
Copy link
Member Author

Hi @zyxue! We're trying to get a release of ADAM out tomorrow, and we wanted to get your work on #1185 in to that release, so I added a commit here to address @heuermh's comments. Thanks again for all your hard work on the code!

@zyxue
Copy link
Contributor

zyxue commented Oct 13, 2016

I see, I can try to incorporate them tonight (Pacific time). I think @heuermh has some good comment that's worth looking into. You can run a test later, if that's not too late?

@fnothaft
Copy link
Member Author

Squashed and merged as df00eff. Thanks @zyxue for the contribution!

@fnothaft fnothaft closed this Oct 13, 2016
@fnothaft fnothaft deleted the zyxue-FastqRecordConverter branch October 13, 2016 23:03
@fnothaft
Copy link
Member Author

I see, I can try to incorporate them tonight (Pacific time). I think @heuermh has some good comment that's worth looking into. You can run a test later, if that's not too late?

Not to worry! I was able to address @heuermh's comments here (and test them as well) real quick. Thanks again for the contribution; we appreciate you taking the time to contribute the code and are glad that you were able to patch up this issue for us!

@zyxue
Copy link
Contributor

zyxue commented Oct 13, 2016

OK, Thanks! ADAM is a good project! I am glad that I can contribute.

@heuermh
Copy link
Member

heuermh commented Oct 14, 2016

@zyxue thank you for your contribution, and thank you for your patience through the process, sometimes Jenkins can be a bit flaky.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants