Ported GATK's FastaReferenceWriter #1172

yfarjoun · 2018-08-08T21:35:49Z

Implemented some basic validation functionality rather than stripping it out of the implementation of FastaReferenceWriter)
Implemented a simple version of a RandomDNA creator (and modified SAMRecordSetBuilder to use it)
Ported the tests (for FastaReferenceWriter) from GATK.
Replaces Class for writing fasta format file. #1058

Description

Please explain the changes you made here.
Explain the motivation for making this change. What existing problem does the pull request solve?

Checklist

Code compiles correctly
New tests covering changes and new functionality
All tests passing
Extended the README / documentation, if necessary
Is not backward compatible (breaks binary or source compatibility)

yfarjoun · 2018-08-08T21:36:15Z

@alecw will this work as well for you?

codecov-io · 2018-08-08T21:50:58Z

Codecov Report

Merging #1172 into master will increase coverage by 1.963%.
The diff coverage is 79.111%.

@@               Coverage Diff               @@
##              master     #1172       +/-   ##
===============================================
+ Coverage     68.343%   70.306%   +1.963%     
- Complexity      8014      9891     +1877     
===============================================
  Files            541       611       +70     
  Lines          32716     38378     +5662     
  Branches        5531      6717     +1186     
===============================================
+ Hits           22359     26982     +4623     
- Misses          8136      8894      +758     
- Partials        2221      2502      +281

Impacted Files	Coverage Δ	Complexity Δ
...main/java/htsjdk/samtools/SAMRecordSetBuilder.java	`89.256% <100%> (-0.175%)`	`70 <1> (-1)`
...mtools/reference/ReferenceSequenceFileFactory.java	`78.723% <100%> (ø)`	`22 <1> (ø)`	⬇️
src/main/java/htsjdk/utils/ValidationUtils.java	`40% <40%> (ø)`	`6 <6> (?)`
...c/main/java/htsjdk/samtools/util/SequenceUtil.java	`70.714% <40.909%> (-1.384%)`	`129 <9> (+3)`
...amtools/reference/FastaReferenceWriterBuilder.java	`84.314% <84.314%> (ø)`	`24 <24> (?)`
...tsjdk/samtools/reference/FastaReferenceWriter.java	`91.2% <91.2%> (ø)`	`41 <41> (?)`
src/main/java/htsjdk/samtools/SAMTagUtil.java	`54.444% <0%> (-41.634%)`	`4% <0%> (-2%)`
.../variant/variantcontext/StructuralVariantType.java	`80% <0%> (-20%)`	`2% <0%> (+1%)`
...samtools/cram/encoding/readfeatures/Insertion.java	`34.375% <0%> (-4.514%)`	`8% <0%> (+4%)`
src/main/java/htsjdk/samtools/LinearIndex.java	`56.818% <0%> (-3.182%)`	`20% <0%> (+10%)`
... and 164 more

alecw · 2018-08-08T21:52:01Z

@yfarjoun I'll try this when I get a chance.

magicDGS

Some overall comments about the design (I did not check the implementation yet).

magicDGS · 2018-08-09T07:04:56Z

src/main/java/htsjdk/ValidationUtils.java

+ * THE SOFTWARE.
+ */
+
+package htsjdk;


This can better live in htsjdk.utils

magicDGS · 2018-08-09T07:06:31Z

src/main/java/htsjdk/ValidationUtils.java

+/**
+ * Simple functions that streamline the checking of values.
+ */
+public class ValidationUtils {


Finally a class like this in HTSJDK!

magicDGS · 2018-08-09T07:09:07Z

src/main/java/htsjdk/samtools/reference/FastaReferenceWriter.java

+ * <ul>
+ * <li>Sequence names are valid (non-empty, without space/blank, control characters),</li>
+ * <li>sequence description are valid (without control characters),</li>
+ * <li>bases are valid nucleotides ore IUPAC redundancy codes and X [ACGTNX...] (lower or uppercase are accepted),</li>


typo: nucleotides ore IUPAC -> nucleotides or IUPAC

magicDGS · 2018-08-09T07:10:52Z

src/main/java/htsjdk/samtools/reference/FastaReferenceWriter.java

+     * @throws IllegalArgumentException if {@code fastaFile} is {@code null}.
+     * @throws IOException              if such exception is thrown when accessing the output path resources.
+     */
+    public FastaReferenceWriter(final Path fastaFile, final boolean makeFaiOutput, final boolean makeDictOutput)


Maybe it is better to have a factory instead of too many constructors...

magicDGS · 2018-08-09T07:13:55Z

src/main/java/htsjdk/samtools/reference/FastaReferenceWriter.java

+        // for the sake of avoiding creating output if basesPerLine is invalid.
+        this.defaultBasePerLine = checkBasesPerLine(basesPerLine);
+
+        this.fastaStream = new CountingOutputStream(Files.newOutputStream(fastaFile));


Support for compressed FASTA files (with gzi/fai index) can be also added.

later perhaps...I wanted to get something in that works for the simple case...feel free to modify it afterwards...

Can you please open an issue and mention it here (or in the factory when the stream is open)? It is easier to track that way

magicDGS · 2018-08-09T07:15:11Z

src/main/java/htsjdk/samtools/reference/FastaReferenceWriter.java

+        }
+    }
+
+    private void writeIndexEntry()


Isn't this functionality somewhere already? Otherwise, it will be nice to have an index-codec for decoding/encoding

Or a FastaIndexing class to index on the fly streams

Currently FastaSequenceIndex and FastaSequenceIndexCreator are geared towards making an index from a fasta-file, not for generating the index on the fly....so without some serious refactoring that's not going to be easy.

I open #1173 for the refactor

magicDGS · 2018-08-09T07:20:25Z

src/main/java/htsjdk/samtools/reference/FastaReferenceWriter.java

+     * @param bases       the sequence bases, cannot be {@code null}.
+     * @throws IOException if such exception is thrown when writing in the output resources.
+     */
+    public static void writeSingleSequenceReference(final Path whereTo, final boolean makeIndex,


If there is a factory/builder for the writer, the static methods can be moved there

Personally I would expect a static method like this to be in the class itself, not a factory method.

magicDGS · 2018-08-09T07:21:39Z

src/main/java/htsjdk/utils/NullWriter.java

+/**
+ * Implementation of a writer that does nothing
+ */
+public class NullWriter extends Writer {


Should this be in the main or the test sources?

main....it's used in the production code

I am not sure it belongs in htsjdk and not in your production code.

Until you have more than one use case in htsjdk, it could live inside the writer class as a static private inner class.

magicDGS · 2018-08-09T07:24:19Z

src/test/java/htsjdk/samtools/reference/FastaReferenceWriterTest.java

+import java.util.stream.IntStream;
+import java.util.stream.Stream;
+
+public class FastaReferenceWriterTest extends HtsjdkTest {


Will be nice to have a test that reads an input FASTA, writes it down and checks if the files are the same (either MD5 or re-reading line by line).

nh13

I can review FastaReferenceWriter once the changes @magicDGS suggested about a factory instead of so many public constructors.

nh13 · 2018-08-09T16:18:27Z

src/main/java/htsjdk/ValidationUtils.java

+     */
+    public static <I, T extends Collection<I>> T nonEmpty(T collection, String message){
+        nonNull(collection, "The collection is null: " + message);
+        if(collection.isEmpty()){


){ -> ) { here and throughout. Generally, the spacing in if/else and so on is not consistent here and throughout.

finals can be used here and throughout.

nh13 · 2018-08-09T16:22:10Z

src/main/java/htsjdk/ValidationUtils.java

+     */
+    public static <T> T nonNull(final T object, final Supplier<String> message) {
+        if (object == null) {
+            throw new IllegalArgumentException(message.get());


this and below is ripe for a method like

public static T require(final T actual, final T expected, final Supplier<String> message) { if (actual == expected) return actual; else throw new IllegalArgumentException(message.get()); }

I'd just use require in scala :P

nh13 · 2018-08-09T16:22:34Z

src/main/java/htsjdk/ValidationUtils.java

+        return nonEmpty(collection, "collection must not be null or empty.");
+    }
+
+    public static void validateArg(final boolean condition, final String msg){


docs here and blow

nh13 · 2018-08-09T16:24:01Z

src/main/java/htsjdk/samtools/SAMRecordSetBuilder.java

-        for (int i = 0; i < length; ++i) {
-            bases[i] = BASES[this.random.nextInt(BASES.length)];
-        }
+        final byte[] bases = SequenceUtil.getRandomBases(random, length);


I think you can get rid of BASES now.

nh13 · 2018-08-09T16:25:40Z

src/main/java/htsjdk/samtools/util/SequenceUtil.java

+        }
+
+        return bases;
+    }


Just one test please

done. thanks for the reminder.

nh13 · 2018-08-09T16:26:09Z

src/main/java/htsjdk/utils/NullWriter.java

@@ -0,0 +1,32 @@
+package htsjdk.utils;


nh13 · 2018-08-09T16:26:31Z

src/main/java/htsjdk/utils/NullWriter.java

+/**
+ * Implementation of a writer that does nothing
+ */
+public class NullWriter extends Writer {


I am not sure it belongs in htsjdk and not in your production code.

pshapiro4broad · 2018-08-09T17:46:09Z

src/main/java/htsjdk/samtools/reference/FastaReferenceWriter.java

+
+/**
+ * Writes a FASTA formatted reference file.
+ * <p>


Does this javadoc look correct to you when it's generated? I would change it in a few ways:

omit </p> as it will result in (unnecessary) extra vertical space

for a code block, wrap with <pre>..</pre> with proper indenting

also, </li> is unnecessary although it does no harm to include it

pshapiro4broad · 2018-08-09T17:48:41Z

src/main/java/htsjdk/samtools/reference/FastaReferenceWriter.java

+        this.defaultBasePerLine = checkBasesPerLine(basesPerLine);
+        this.fastaStream = new CountingOutputStream(fastaOutput);
+        this.indexWriter = indexOutput == null ? NullWriter.NULL_WRITER : new OutputStreamWriter(indexOutput, CHARSET);
+        final BufferedWriter dictWriter = new BufferedWriter(dictOutput == null ? NullWriter.NULL_WRITER : new OutputStreamWriter(dictOutput, CHARSET));


Not sure why you have a local variable for this when you can assign to the field directly.

the reason is that this.dictWriter is a Writer while SAMSequenceDictionaryCodec constructor takes a BufferedWriter...I could cast...but that might be uglier.

Ah, I didn't notice that

pshapiro4broad · 2018-08-09T17:50:46Z

src/main/java/htsjdk/samtools/reference/FastaReferenceWriter.java

+
+        this.fastaStream = new CountingOutputStream(Files.newOutputStream(fastaFile));
+        this.indexWriter = indexFile == null ? NullWriter.NULL_WRITER : new OutputStreamWriter(Files.newOutputStream(indexFile), CHARSET);
+        final BufferedWriter dictWriter = new BufferedWriter(dictFile == null ? NullWriter.NULL_WRITER : new OutputStreamWriter(Files.newOutputStream(dictFile), CHARSET));


It would be nice to avoid the code duplication with the other constructor, if possible. Using a factory method or builder pattern is one way to do this. Or you could move the shared code to a private method.

the issue is that we want to check the basesPerLine before the OutputFile is created...and as the comment (in the other constructor says) this is hard to do without code duplication....

Everything else has to be in the constructor since it's final.

nm. changed the signature and I think it works now.

pshapiro4broad · 2018-08-09T17:52:00Z

src/main/java/htsjdk/samtools/reference/FastaReferenceWriter.java

+        this.dictWriter = dictWriter;
+        this.dictCodec = new SAMSequenceDictionaryCodec(dictWriter);
+        this.dictCodec.encodeHeaderLine(false);
+        this.sequenceNamesAndSizes = new LinkedHashMap<>();


It looks like this is always initialized the same way. If so, moving the initialization assignment to the field declaration would make this clearer to the reader.

pshapiro4broad · 2018-08-09T17:54:45Z

src/main/java/htsjdk/samtools/reference/FastaReferenceWriter.java

+    // checks that a sequence name is valid.
+    private static void checkSequenceName(final String name) {
+        ValidationUtils.nonNull(name ,"Sequence name should not be null");
+        ValidationUtils.nonEmpty(name,"Sequence name should not be empty");


missing space after ,

src/main/java/htsjdk/samtools/reference/FastaReferenceWriter.java

pshapiro4broad · 2018-08-09T18:26:17Z

src/main/java/htsjdk/utils/ValidationUtils.java

+     */
+    public static <I, T extends Collection<I>> T nonEmpty(T collection, String message){
+        nonNull(collection, "The collection is null: " + message);
+        if(collection.isEmpty()){


add spaces before/after ()

pshapiro4broad · 2018-08-09T18:26:35Z

src/main/java/htsjdk/utils/ValidationUtils.java

+     * @return the original collection
+     * @throws IllegalArgumentException if collection is null or empty
+     */
+    public static <I, T extends Collection<I>> T nonEmpty(T collection, String message){


add space before {. many missing spaces in this file, would be easier to re-format in the IDE to fix them all

pshapiro4broad · 2018-08-09T18:27:44Z

src/main/java/htsjdk/utils/ValidationUtils.java

+     * @return the original collection
+     * @throws IllegalArgumentException if collection is null or empty
+     */
+    public static <I, T extends Collection<I>> T nonEmpty(T collection, String message){


Is it necessary to parameterize on I here? Is <T extends Collection<?>> not sufficient?

pshapiro4broad · 2018-08-09T18:28:12Z

src/main/java/htsjdk/utils/ValidationUtils.java

+        nonNull(collection, "The collection is null: " + message);
+        if(collection.isEmpty()){
+            throw new IllegalArgumentException("The collection is empty: " + message);
+        } else {


unnecessary else after throw

pshapiro4broad · 2018-08-09T18:30:05Z

src/main/java/htsjdk/utils/ValidationUtils.java

+     * @param collection any Collection
+     * @return true if the collection exists and has elements
+     */
+    public static boolean isNonEmpty(Collection<?> collection){


Seems confusing to me to have two different styles of validation here that do pretty much the same thing.

pshapiro4broad · 2018-08-10T14:47:05Z

src/main/java/htsjdk/samtools/reference/FastaReferenceWriter.java

+        }
+        currentSequenceName = sequenceName;
+        currentBasesPerLine = basesPerLine;
+        final StringBuilder builder = new StringBuilder(sequenceName.length() + nonNullDescription.length() + 10);


It looks like what you really want is sequenceName.length() + nonNullDescription.length() + 2. In practice does it make much difference (regarding code performance) to preallocate here?

I don't know about the performance benefit. I think that the idea is to preallocate if can.. but you are correct about the +2 (I think)

yfarjoun · 2018-08-10T17:19:24Z

ok @nh13 it's using a builder now. :-)

nh13

@yfarjoun I have stopped reviewing the FastaReferenceWriter class as I think the API needs a lot of work to leverage the various htsjdk classes and make it a lot friendlier. It really feels like a quick port.

I'd like to see the public methods accept htsjdk classes rather than byte arrays, strings, and other things. If there is a reason to bass those directly, for example, because a contig is too big, I can understand that, but I feel like then the API can be improved.

nh13 · 2018-08-10T20:24:14Z

src/main/java/htsjdk/samtools/reference/FastaReferenceWriter.java

+    protected FastaReferenceWriter(final int basesPerLine,
+                                final OutputStream fastaOutput,
+                                final OutputStream indexOutput,
+                                final OutputStream dictOutput) {


ident is wrong

nh13 · 2018-08-10T20:24:43Z

src/main/java/htsjdk/samtools/reference/FastaReferenceWriter.java

+     * @param dictOutput  the output stream to the dictFile, if requested, {@code null} if none should be generated.
+     * @throws IllegalArgumentException if {@code fastaFile} is {@code null} or {@code basesPerLine} is 0 or negative.
+     */
+    protected FastaReferenceWriter(final int basesPerLine,


Cant this be defaulted to DEFAULT_BASES_PER_LINE?

in the factory it does that.

nh13 · 2018-08-10T20:25:40Z

src/main/java/htsjdk/samtools/reference/FastaReferenceWriter.java

+            } else if (Character.isISOControl(ch)) {
+                throw new IllegalArgumentException("the input name contains control characters: '" + name + "'");
+            }
+        }


If you stored SAMSequenceRecords this would take care of itself!

yeah, but I can't use that and still get things like description (which SAMSequenceRecord doesn't support) and also incremental buildup of a sequence..

nh13 · 2018-08-10T20:26:12Z

src/main/java/htsjdk/samtools/reference/FastaReferenceWriter.java

+     * The value is the sequence length in bases.
+     * </p>
+     */
+    private final Map<String, Long> sequenceNamesAndSizes = new LinkedHashMap<>();


Can't you use a Map<String, SAMSequenceRecord>?

nh13 · 2018-08-10T20:26:38Z

src/main/java/htsjdk/samtools/reference/FastaReferenceWriter.java

+    protected FastaReferenceWriter(final int basesPerLine,
+                                final OutputStream fastaOutput,
+                                final OutputStream indexOutput,
+                                final OutputStream dictOutput) {


It'd be great if index/dict were optional.

they are (in builder)

src/main/java/htsjdk/samtools/reference/FastaReferenceWriter.java

nh13 · 2018-08-10T20:32:42Z

src/main/java/htsjdk/samtools/reference/FastaReferenceWriter.java

+        return appendBases(bases, 0, bases.length);
+    }
+
+    /**


I've stopped reviewing, because I just don't understand why this class doesn't leverage the various htsjdk classes for it's API. For example, ReferenceSequence and SAMSequenceRecord. I get wanting to feed sub-parts of the reference FASTA at a time, but can't that be handled by giving it an iterator over sub-sequences that's evaluated lazily?

That sounds nice...but not a simple port. I was looking for something so that folks could write FASTA files with htsjdk.....however, I've realized that the length isn't used...so this is a set of names...I'll make that change.

magicDGS

I did not review the code (again) because I do not have too much time, but I am concern about users passing compressed streams if they do not work.

I agree with @nh13 about using also HTSJDK objects for the writer for better integration.

magicDGS · 2018-08-13T14:50:01Z

src/main/java/htsjdk/samtools/reference/FastaReferenceWriter.java

+     * You can specify a specific output stream to each file: the main fasta output, its index and its dictionary.
+     * </p>
+     *
+     * @param fastaOutput the output fasta file path.


Can this stream be passed as a bgzip compressed one, and in that case will it work properly? That should be specify here to help developers to refactor to include bgzip-support.

magicDGS · 2018-08-13T14:51:57Z

src/main/java/htsjdk/samtools/reference/FastaReferenceWriter.java

+     *
+     * @param fastaOutput the output fasta file path.
+     * @param indexOutput the output stream to the index file, if requested, {@code null} if none should be generated.
+     * @param dictOutput  the output stream to the dictFile, if requested, {@code null} if none should be generated.


Specify that both index/dict streams should not be compressed (the same in the builder).

alecw · 2018-08-21T20:48:11Z

Hi @yfarjoun,
I did a quick test. Works for me.
Thanks!

- Implemented some basic validation functionality rather than stripping it out of the implementation of FastaReferenceWriter) - Implemented a simple version of a RandomDNA creator (and modified SAMRecordSetBuilder to use it) - Ported the tests (for FastaReferenceWriter) from GATK.

…elliJ!)

lbergelson

Seems pretty good to me. Some minor comments and then I think 👍.

lbergelson · 2018-09-24T18:44:11Z

src/main/java/htsjdk/samtools/reference/FastaReferenceWriterBuilder.java

+     * @param fastaFile a {@link File} to the output fasta file.
+     * @return this builder
+     */
+    public FastaReferenceWriterBuilder setFastaFile(File fastaFile) {


I would just not add methods that take File

lbergelson · 2018-09-24T18:46:47Z

src/main/java/htsjdk/samtools/reference/FastaReferenceWriterBuilder.java

+
+    /**
+     * Sets whether to automatically generate an dictionary file from the name of the fasta-file (assuming it is given
+     * as a file). This can only happen if both the index file and output stream are null.


This should probably say dict here.

lbergelson · 2018-09-24T18:55:00Z

src/main/java/htsjdk/samtools/reference/FastaReferenceWriterBuilder.java

+        if (indexFile == null && indexOutput == null) {
+            indexFile = defaultFaiFile(makeFaiOutput, fastaFile);
+        }
+        if (dictFile == null && dictOutput == null) {


I think we should detect and error in the case where more than one of these is specified / if one is specified but it's set to not output.

- adding a test - fixed a bug - removed final from TWR, since its unneeded

yfarjoun · 2018-10-23T17:46:10Z

@lbergelson back to you.

src/main/java/htsjdk/samtools/reference/FastaReferenceWriter.java

yfarjoun · 2018-12-03T19:41:24Z

src/main/java/htsjdk/samtools/reference/FastaReferenceWriter.java

+     */
+    public FastaReferenceWriter appendString(final String basesString)
+            throws IOException {
+        return appendBases(basesString.getBytes());


specify encoding ascii

src/main/java/htsjdk/samtools/reference/FastaReferenceWriter.java

yfarjoun · 2018-12-12T14:24:42Z

@droazen, please take another look.

yfarjoun · 2019-02-01T21:20:43Z

merging due to thumb from louis.

yfarjoun mentioned this pull request Aug 8, 2018

Class for writing fasta format file. #1058

Closed

5 tasks

magicDGS reviewed Aug 9, 2018

View reviewed changes

nh13 requested changes Aug 9, 2018

View reviewed changes

pshapiro4broad reviewed Aug 9, 2018

View reviewed changes

magicDGS mentioned this pull request Aug 10, 2018

Refactor FASTA indexing to share implementation in input/output #1173

Open

pshapiro4broad reviewed Aug 10, 2018

View reviewed changes

nh13 requested changes Aug 10, 2018

View reviewed changes

magicDGS reviewed Aug 13, 2018

View reviewed changes

Yossi Farjoun added 8 commits September 5, 2018 13:38

- addressed some review comments.

d7583f5

- addressed more review comments.

9f83c81

- Due to popular demand, I converted to a builder pattern (thanks Int…

2b5e54f

…elliJ!)

- start was missing final 't' in more than one place...

2ebc96f

- javaDoc

7d1e457

-responding to comments

a4be8c8

- Adding comment regarding compressed files and streams

98ada8d

yfarjoun force-pushed the yf_FastaWriter_fromGATK branch from a77e99e to 98ada8d Compare September 5, 2018 17:39

- oops

ea17a7f

yfarjoun added the Waiting for Review This PR is waiting for a reviewer to respond label Sep 10, 2018

lbergelson approved these changes Sep 24, 2018

View reviewed changes

yfarjoun self-assigned this Oct 1, 2018

yfarjoun added Waiting for revisions This PR has received comments from reviewers and is waiting for the Author to respond and removed Waiting for Review This PR is waiting for a reviewer to respond labels Oct 1, 2018

- responding to review

308632a

Yossi Farjoun added 3 commits October 23, 2018 11:51

- responding to review

1119134

- adding a test - fixed a bug - removed final from TWR, since its unneeded

- fixing bug in test

add3333

- fixing bug in test

6336bea

yfarjoun added Waiting for Review This PR is waiting for a reviewer to respond and removed Waiting for revisions This PR has received comments from reviewers and is waiting for the Author to respond labels Oct 23, 2018

yfarjoun commented Dec 3, 2018

View reviewed changes

src/main/java/htsjdk/samtools/reference/FastaReferenceWriter.java Outdated Show resolved Hide resolved

yfarjoun commented Dec 3, 2018

View reviewed changes

src/main/java/htsjdk/samtools/reference/FastaReferenceWriter.java Outdated Show resolved Hide resolved

- responding to review comments

412c6e8

yfarjoun merged commit 942e3d6 into master Feb 1, 2019

yfarjoun deleted the yf_FastaWriter_fromGATK branch February 1, 2019 21:21

Ported GATK's FastaReferenceWriter #1172

Ported GATK's FastaReferenceWriter #1172

Conversation

yfarjoun commented Aug 8, 2018

Description

Checklist

yfarjoun commented Aug 8, 2018

codecov-io commented Aug 8, 2018 • edited Loading

Codecov Report

alecw commented Aug 8, 2018

magicDGS left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

magicDGS Aug 9, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yfarjoun Aug 9, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nh13 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yfarjoun commented Aug 10, 2018

nh13 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

magicDGS left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alecw commented Aug 21, 2018

lbergelson left a comment

Choose a reason for hiding this comment

codecov-io commented Aug 8, 2018 •

edited

Loading

magicDGS Aug 9, 2018 •

edited

Loading

yfarjoun Aug 9, 2018 •

edited

Loading

yfarjoun Dec 3, 2018 •

edited

Loading