Add option to return sequences from the Genbank file as a stream of sequences#870
Closed
emckee2006 wants to merge 3 commits intobiojava:masterfrom
Closed
Add option to return sequences from the Genbank file as a stream of sequences#870emckee2006 wants to merge 3 commits intobiojava:masterfrom
emckee2006 wants to merge 3 commits intobiojava:masterfrom
Conversation
josemduarte
requested changes
Apr 15, 2020
Contributor
josemduarte
left a comment
josemduarte
left a comment
There was a problem hiding this comment.
Thanks! One comment about error handling
| } catch (IOException | CompoundNotFoundException e) { | ||
| // TODO Auto-generated catch block | ||
| e.printStackTrace(); | ||
| return null; |
Contributor
There was a problem hiding this comment.
I'd rather throw the exception forward and let the caller handle it. Returning null means all callers have to handle the null and that they lose the ability to provide a good error message.
Contributor
|
Could you merge master into your branch? The tests should pass then. A problem with them was fixed in #871 |
Contributor
Author
|
I thought I had... I'll double check.
As far as the exception goes, can you actually pass that exception on? If I
try, Eclipse tells me it's an error...
…On Wed, Apr 15, 2020, 3:39 PM Jose Manuel Duarte ***@***.***> wrote:
Could you merge master into your branch? The tests should pass then. A
problem with them was fixed in #871
<#871>
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#870 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABLFC4FBPELB7NPSY7DUQ3LRMYEOZANCNFSM4MIQXC4A>
.
|
Contributor
Author
|
I successfully merged the new stuff in.
There are ways to get the exception out of the lambda. They are a bit
tedious, but if it is helpful, I can give that a try...
…On Wed, Apr 15, 2020, 4:02 PM Erik McKee ***@***.***> wrote:
I thought I had... I'll double check.
As far as the exception goes, can you actually pass that exception on? If
I try, Eclipse tells me it's an error...
On Wed, Apr 15, 2020, 3:39 PM Jose Manuel Duarte ***@***.***>
wrote:
> Could you merge master into your branch? The tests should pass then. A
> problem with them was fixed in #871
> <#871>
>
> —
> You are receiving this because you authored the thread.
> Reply to this email directly, view it on GitHub
> <#870 (comment)>, or
> unsubscribe
> <https://github.com/notifications/unsubscribe-auth/ABLFC4FBPELB7NPSY7DUQ3LRMYEOZANCNFSM4MIQXC4A>
> .
>
|
Contributor
|
There seems to be an issue with the new test. See logs from travis, there's a timeout after running GenbankReaderTest |
Contributor
Author
|
I'll take a look... if I run it in junit in eclipse it runs fine... that's
odd
…On Sun, Apr 19, 2020, 6:40 PM Jose Manuel Duarte ***@***.***> wrote:
There seems to be an issue with the new test. See logs from travis,
there's a timeout after running GenbankReaderTest
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#870 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABLFC4CKWIYXS7Z365VEHXDRNN4VJANCNFSM4MIQXC4A>
.
|
Contributor
Author
|
How do i rerun Travis after adding a new commit? (or what is a better debugging mechanism here?) |
Contributor
|
I've just triggered the travis tests again |
Contributor
Author
|
I commented out the new test to verify that it is the cause. Is there an
easy way to debug?
…On Tue, Apr 21, 2020, 1:41 PM Jose Manuel Duarte ***@***.***> wrote:
I've just triggered the travis tests again
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#870 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABLFC4HHRK6FTP2BQZKPBW3RNXLFFANCNFSM4MIQXC4A>
.
|
Contributor
Author
|
OK, it still hangs even if not executing my test... |
Member
Can you try to run |
lafita
requested changes
Apr 22, 2020
|
|
||
| @Test | ||
| public void testSequenceStream() { | ||
| CheckableInputStream inStream = new CheckableInputStream(this.getClass().getResourceAsStream("/two-dnaseqs.gb")); |
Member
There was a problem hiding this comment.
Did you also include this file in the test resources directory? It seems not to be in this PR
| LinkedHashMap<String,S> sequences = new LinkedHashMap<>(); | ||
| @SuppressWarnings("unchecked") | ||
| int i=0; | ||
| while(true) { |
Member
There was a problem hiding this comment.
This is what is hanging the build. Because the stream returned is null (the file does not exist, see comment below), sequence is always null and this while loop never finishes.
The while loop is not needed anymore, simply take an action if the sequence is null.
Contributor
Author
|
Isn't that the same file used by one of the existing tests?
…On Wed, Apr 22, 2020, 6:41 AM Aleix Lafita ***@***.***> wrote:
***@***.**** requested changes on this pull request.
------------------------------
In
biojava-core/src/test/java/org/biojava/nbio/core/sequence/io/GenbankReaderTest.java
<#870 (comment)>:
> @@ -163,6 +164,27 @@ public void testProcess() throws Exception {
assertEquals(3, dnaSequence.getAccession().getVersion().intValue());
assertTrue(genbankDNA.isClosed());
}
+
+ @test
+ public void testSequenceStream() {
+ CheckableInputStream inStream = new CheckableInputStream(this.getClass().getResourceAsStream("/two-dnaseqs.gb"));
Did you also include this file in the test resources directory? It seems
not to be in this PR
------------------------------
In
biojava-core/src/main/java/org/biojava/nbio/core/sequence/io/GenbankReader.java
<#870 (comment)>:
> @@ -153,35 +154,56 @@ public GenbankReader(
}
LinkedHashMap<String,S> sequences = new LinkedHashMap<>();
- @SuppressWarnings("unchecked")
int i=0;
while(true) {
This is what is hanging the build. Because the stream returned is null
(the file does not exist, see comment below), sequence is always null and
this while loop never finishes.
The while loop is not needed anymore, simply take an action if the
sequence is null.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#870 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABLFC4CR43PCDWUP4RHOMALRN3CVRANCNFSM4MIQXC4A>
.
|
Member
|
True the file exists, but the while loop is not needed anymore and probably the one causing the timeout in travis. @emckee2006 does maven install work locally for you? |
Contributor
Author
|
Why doesn't that method need the while loop? It still needs to support
grabbing multiple sequences...
…On Wed, Apr 22, 2020, 10:55 AM Aleix Lafita ***@***.***> wrote:
True the file exists, but the while loop is not needed anymore and
probably the one causing the timeout in travis. @emckee2006
<https://github.com/emckee2006> does maven install work locally for you?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#870 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABLFC4ELVXNIZIJCRTIS4Z3RN4AP3ANCNFSM4MIQXC4A>
.
|
Contributor
Author
|
This needs help... closing for now... |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.