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

Refactoring `org.bdgenomics.adam.io` package. #1064

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
3 participants
@fnothaft
Member

fnothaft commented Jul 3, 2016

  • Added documentation to all classes, methods, and fields that were missing descriptions.
  • Factored duplicated code from SingleFastqRecordReader and InterleavedFastqRecordReader out into a new abstract base class FastqRecordReader.
  • Tightened up access modifiers where possible. Sadly, the two input formats cannot be made more private, since Java has a more restricted form of package-private than scala, and the input formats are used from inside of org.bdgenomics.adam.rdd.
  • Removed @author tags.
@AmplabJenkins

This comment has been minimized.

Show comment
Hide comment
@AmplabJenkins

AmplabJenkins Jul 3, 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/1295/
Test PASSed.

AmplabJenkins commented Jul 3, 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/1295/
Test PASSed.

* and the quality encoding includes '@' in its valid character range. So how should one
* distinguish between \n@ as a record delimiter and and \n@ as part of a multi-line
* quality string?
*

This comment has been minimized.

@heuermh

heuermh Jul 6, 2016

Member

Yes

@heuermh
Show outdated Hide outdated adam-core/src/main/java/org/bdgenomics/adam/io/FastqRecordReader.java
* to the underlying file system.
* @param split The file split to read.
*/
public FastqRecordReader(Configuration conf, FileSplit split) throws IOException {

This comment has been minimized.

@heuermh

heuermh Jul 6, 2016

Member

final Configuration conf, final FileSplite split

@heuermh

heuermh Jul 6, 2016

Member

final Configuration conf, final FileSplite split

Show outdated Hide outdated adam-core/src/main/java/org/bdgenomics/adam/io/FastqRecordReader.java
FSDataInputStream fileIn = fs.open(file);
CompressionCodecFactory codecFactory = new CompressionCodecFactory(conf);
CompressionCodec codec = codecFactory.getCodec(file);

This comment has been minimized.

@heuermh

heuermh Jul 6, 2016

Member

Not very java-ish formatting

@heuermh

heuermh Jul 6, 2016

Member

Not very java-ish formatting

Show outdated Hide outdated adam-core/src/main/java/org/bdgenomics/adam/io/FastqRecordReader.java
* have read so far out of the total bytes to read.
*/
public float getProgress() {
if (start == end)

This comment has been minimized.

@heuermh

heuermh Jul 6, 2016

Member

add squigglys

@heuermh

heuermh Jul 6, 2016

Member

add squigglys

Show outdated Hide outdated adam-core/src/main/java/org/bdgenomics/adam/io/FastqRecordReader.java
readName.clear();
long skipped = appendLineInto(readName, true);
pos += skipped;
if (skipped == 0)

This comment has been minimized.

@heuermh

heuermh Jul 6, 2016

Member

... and here

@heuermh

heuermh Jul 6, 2016

Member

... and here

Show outdated Hide outdated adam-core/src/main/java/org/bdgenomics/adam/io/FastqRecordReader.java
* @throws EOFException Throws if eofOk was false and we hit an EOF in
* the current line.
*/
private int appendLineInto(Text dest, boolean eofOk) throws EOFException, IOException {

This comment has been minimized.

@heuermh

heuermh Jul 6, 2016

Member

final Text dest, final boolean eof0k

@heuermh

heuermh Jul 6, 2016

Member

final Text dest, final boolean eof0k

Show outdated Hide outdated adam-core/src/main/java/org/bdgenomics/adam/io/SingleFastqInputFormat.java
*/
private static class SingleFastqRecordReader extends FastqRecordReader {
public SingleFastqRecordReader(Configuration conf,

This comment has been minimized.

@heuermh

heuermh Jul 6, 2016

Member

private class shouldn't need a pubilc ctr?

@heuermh

heuermh Jul 6, 2016

Member

private class shouldn't need a pubilc ctr?

@heuermh heuermh referenced this pull request Jul 6, 2016

Closed

Release ADAM version 0.20.0 #1048

47 of 61 tasks complete
Refactoring `org.bdgenomics.adam.io` package.
* Added documentation to all classes, methods, and fields that were missing
  descriptions.
* Factored duplicated code from `SingleFastqRecordReader` and
  `InterleavedFastqRecordReader` out into a new abstract base class
  `FastqRecordReader`.
* Tightened up access modifiers where possible. Sadly, the two input formats
  cannot be made more private, since Java has a more restricted form of
  package-private than scala, and the input formats are used from inside of
  `org.bdgenomics.adam.rdd`.
* Removed `@author` tags.
@fnothaft

This comment has been minimized.

Show comment
Hide comment
@fnothaft

fnothaft Jul 7, 2016

Member

Addressed review comments.

Member

fnothaft commented Jul 7, 2016

Addressed review comments.

@AmplabJenkins

This comment has been minimized.

Show comment
Hide comment
@AmplabJenkins

AmplabJenkins Jul 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/1319/
Test PASSed.

AmplabJenkins commented Jul 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/1319/
Test PASSed.

@AmplabJenkins

This comment has been minimized.

Show comment
Hide comment
@AmplabJenkins

AmplabJenkins Jul 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/1320/
Test PASSed.

AmplabJenkins commented Jul 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/1320/
Test PASSed.

@heuermh

This comment has been minimized.

Show comment
Hide comment
@heuermh

heuermh Jul 7, 2016

Member

LGTM

Member

heuermh commented Jul 7, 2016

LGTM

@heuermh

This comment has been minimized.

Show comment
Hide comment
@heuermh

heuermh Jul 7, 2016

Member

Merged commit 40ee638

Thank you, @fnothaft!

Member

heuermh commented Jul 7, 2016

Merged commit 40ee638

Thank you, @fnothaft!

@heuermh heuermh closed this Jul 7, 2016

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