Small improvements #829

lee-mcfaul · 2019-02-18T22:05:26Z

Although I couldn't reproduce the original error in #800 (I suspect that maybe the user didn't have a version of the libraries that included the fix in #666 ) I followed the method documentation and moved the close action to process() from process(int) since process(int) should never close the resource under any circumstances.

Also verified that the underlying InputStream was open and closed at appropriate steps.

Used streams to reduce complexity of a double nested for loop.(If that's frowned upon I'll change it back)

Tweaked documentation to use links when referring to library classes.

Although I couldn't reproduce the original error. I followed the method documentation and moved the close action to process() from process(int) since process(int) should never close the resource under any circumstances anyhow. Also went out of scope a little and verified that the underlying InputStream was open and closed at appropriate steps. Used streams to reduce complexity of a double nested for loop.(If that's frowned upon I'll change it back) Tweaked documentation to use links when refering to library classes.

heuermh

minor code style suggestions

biojava-core/src/main/java/org/biojava/nbio/core/sequence/io/GenbankReader.java

biojava-core/src/test/java/org/biojava/nbio/core/sequence/io/GenbankReaderTest.java

biojava-core/src/main/java/org/biojava/nbio/core/sequence/io/GenbankReader.java

Added braces to if statement Removed main method

Made style change to isClosed() method

lee-mcfaul · 2019-02-19T19:34:38Z

Looks like the failing test is in TestUncompressInputStream.java. Which is also failing in master. I compared the original input and the processed array to file and they appear identical at first glance. I'll poke around and see if I can't find out wha't causing the issue.

`java.lang.AssertionError: array lengths differed, expected.length=9068 actual.length=9257

at org.junit.Assert.fail(Assert.java:88)
at org.junit.internal.ComparisonCriteria.assertArraysAreSameLength(ComparisonCriteria.java:76)
at org.junit.internal.ComparisonCriteria.arrayEquals(ComparisonCriteria.java:37)
at org.junit.Assert.internalArrayEquals(Assert.java:532)
at org.junit.Assert.assertArrayEquals(Assert.java:341)
at org.junit.Assert.assertArrayEquals(Assert.java:352)
at org.biojava.nbio.core.util.TestUncompressInputStream.testUncompression(TestUncompressInputStream.java:70)

lee-mcfaul · 2019-02-19T20:08:02Z

It's the line breaks in 'org/biojava/nbio/core/util/build.xml': they're set to windows line breaks instead of linux ones. I'll put in a pull request.

…putStream test pass

lee-mcfaul · 2019-02-19T21:01:17Z

That test now passes... however we got 429 (too many requests) response codes in other tests.

josemduarte · 2019-02-19T22:52:46Z

Good catch @lee-mcfaul ! Thanks for fixing the test. It might have broken when re-formatting all code with an automated script in a recent release.

I did go through the same fix before that (see e78ea4c)

josemduarte

Looks good to me, thank you! Special thanks for improving docs

Normally I would have opened a new branch. but this issue was causing this pull to fail it's build so I'm doing both in one fel swoop. I put the target files in the resource directory and manually copy them over before each test into the temporary working directory which is scanned before calling out in case the file is already there. this should fix the 429 errors in this class specifically.

Fixing missing slash on the travis build server (works without fix on windows but added check for backslash anyhow)

lee-mcfaul · 2019-02-22T04:10:34Z

Unit tests passing now. Integration tests failed with a closed ssh connection https://travis-ci.org/biojava/biojava/jobs/496850065

josemduarte

Thank you again. I think the extra commons-io library is not necessary here.

...core/src/test/java/org/biojava/nbio/core/sequence/loader/GenbankProxySequenceReaderTest.java

josemduarte · 2019-02-22T15:27:56Z

I've restarted the build. That ssl connection is another external resource that's flaky

Removed use of apache io library

typo

lee-mcfaul · 2019-02-23T18:31:44Z

This is turning into a rabbit hole. Any objections to me going through the integration tests, adding any quick fixes to this pull request and setting more complex issues to ignore and creating issues for them?

josemduarte

Thanks, the test changes are great to avoid using the external resource.

I have just one more comment about where to store the new resource files.

...core/src/test/java/org/biojava/nbio/core/sequence/loader/GenbankProxySequenceReaderTest.java

...re/src/test/resources/org/biojava/nbio/core/sequence/GenbankProxySequenceReader/152970917.gb

josemduarte · 2019-02-26T00:46:32Z

Regarding fixing other tests: it'd be great if you could help there and remove as many dependencies on external resources as possible. But it'd be better to do that in separate PRs. The issue that should be referenced is #606

lee-mcfaul · 2019-03-01T00:55:37Z

alright, build succeeded. backing away from this pr slowly. I'll have a peek at #606 next

josemduarte

Looks great, thanks again!

josemduarte · 2019-03-01T00:59:52Z

@heuermh could you give this another look and see if you are happy with it?

josemduarte · 2019-03-07T23:18:21Z

I will merge this in tomorrow, unless someone says something against it.

heuermh requested changes Feb 19, 2019

View reviewed changes

lee-mcfaul added 2 commits February 19, 2019 12:30

Made requested changes to imports.

c77515e

Added braces to if statement Removed main method

Made requested changes to imports in test class.

7601f17

Made style change to isClosed() method

Changing line end for xml to Unix format to make the TestUncompressIn…

9197e89

…putStream test pass

josemduarte approved these changes Feb 19, 2019

View reviewed changes

lee-mcfaul added 2 commits February 21, 2019 22:26

biojava#831

c6f17ca

Fixing missing slash on the travis build server (works without fix on windows but added check for backslash anyhow)

josemduarte requested changes Feb 22, 2019

View reviewed changes

...core/src/test/java/org/biojava/nbio/core/sequence/loader/GenbankProxySequenceReaderTest.java Outdated Show resolved Hide resolved

lee-mcfaul added 2 commits February 22, 2019 21:04

biojava#831

e221bd5

Removed use of apache io library

biojava#831

b652a43

typo

josemduarte requested changes Feb 26, 2019

View reviewed changes

...core/src/test/java/org/biojava/nbio/core/sequence/loader/GenbankProxySequenceReaderTest.java Outdated Show resolved Hide resolved

...re/src/test/resources/org/biojava/nbio/core/sequence/GenbankProxySequenceReader/152970917.gb Show resolved Hide resolved

made requested changes

4fd4149

josemduarte approved these changes Mar 1, 2019

View reviewed changes

josemduarte merged commit 323cd9f into biojava:master Mar 8, 2019

Small improvements #829

Small improvements #829

Uh oh!

Conversation

lee-mcfaul commented Feb 18, 2019

Uh oh!

heuermh left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lee-mcfaul commented Feb 19, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lee-mcfaul commented Feb 19, 2019

Uh oh!

lee-mcfaul commented Feb 19, 2019

Uh oh!

josemduarte commented Feb 19, 2019

Uh oh!

josemduarte left a comment

Choose a reason for hiding this comment

Uh oh!

lee-mcfaul commented Feb 22, 2019

Uh oh!

josemduarte left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

josemduarte commented Feb 22, 2019

Uh oh!

lee-mcfaul commented Feb 23, 2019

Uh oh!

josemduarte left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

josemduarte commented Feb 26, 2019

Uh oh!

lee-mcfaul commented Mar 1, 2019

Uh oh!

josemduarte left a comment

Choose a reason for hiding this comment

Uh oh!

josemduarte commented Mar 1, 2019

Uh oh!

josemduarte commented Mar 7, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

lee-mcfaul commented Feb 19, 2019 •

edited

Loading