Add option to return sequences from the Genbank file as a stream of sequences by emckee2006 · Pull Request #870 · biojava/biojava · GitHub

No description provided.

…equences

Thanks! One comment about error handling

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.

Could you merge master into your branch? The tests should pass then. A problem with them was fixed in #871

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> .

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> > . >

There seems to be an issue with the new test. See logs from travis, there's a timeout after running GenbankReaderTest

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> .

How do i rerun Travis after adding a new commit? (or what is a better debugging mechanism here?)

I've just triggered the travis tests again

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> .

OK, it still hangs even if not executing my test...

I commented out the new test to verify that it is the cause. Is there an easy way to debug?

Can you try to run mvn install locally from the command line or using Maven Install in eclipse?

Did you also include this file in the test resources directory? It seems not to be in this PR

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.

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> .

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?

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> .

This needs help... closing for now...

emckee2006 added 2 commits April 15, 2020 08:07

Add option to return sequences from the Genbank file as a stream of s…

e9fec3c

…equences

Merge branch 'GenbankReader_SequenceStream'

4eb1c20

emckee2006 requested review from josemduarte and sbliven April 15, 2020 12:12

josemduarte requested changes Apr 15, 2020

View reviewed changes

Merge branch 'upstream_master'

7327923

lafita requested changes Apr 22, 2020

View reviewed changes

emckee2006 closed this May 3, 2020

+              			} catch (IOException | CompoundNotFoundException e) {
+              				// TODO Auto-generated catch block
+              				e.printStackTrace();
+              				return null;

+              	@Test
+              	public void testSequenceStream() {
+              		CheckableInputStream inStream = new CheckableInputStream(this.getClass().getResourceAsStream("/two-dnaseqs.gb"));

               		LinkedHashMap<String,S> sequences = new LinkedHashMap<>();
-              		@SuppressWarnings("unchecked")
               		int i=0;
               		while(true) {

Add option to return sequences from the Genbank file as a stream of sequences by emckee2006 · Pull Request #870 · biojava/biojava · GitHub

Add option to return sequences from the Genbank file as a stream of sequences - #870

Add option to return sequences from the Genbank file as a stream of sequences#870emckee2006 wants to merge 3 commits intobiojava:masterbiojava/biojava:masterfrom emckee2006:masteremckee2006/biojava:masterCopy head branch name to clipboard

Conversation

emckee2006 commented Apr 15, 2020

Uh oh!

josemduarte left a comment

Choose a reason for hiding this comment

Uh oh!

josemduarte Apr 15, 2020

Choose a reason for hiding this comment

Uh oh!

josemduarte commented Apr 15, 2020

Uh oh!

emckee2006 commented Apr 15, 2020 via email

Uh oh!

emckee2006 commented Apr 15, 2020 via email

Uh oh!

josemduarte commented Apr 19, 2020

Uh oh!

emckee2006 commented Apr 19, 2020 via email

Uh oh!

emckee2006 commented Apr 21, 2020

Uh oh!

josemduarte commented Apr 21, 2020

Uh oh!

emckee2006 commented Apr 21, 2020 via email

Uh oh!

emckee2006 commented Apr 21, 2020

Uh oh!

lafita commented Apr 22, 2020

Uh oh!

lafita Apr 22, 2020

Choose a reason for hiding this comment

Uh oh!

lafita Apr 22, 2020

Choose a reason for hiding this comment

Uh oh!

emckee2006 commented Apr 22, 2020 via email

Uh oh!

lafita commented Apr 22, 2020

Uh oh!

emckee2006 commented Apr 22, 2020 via email

Uh oh!

emckee2006 commented May 3, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Add option to return sequences from the Genbank file as a stream of sequences#870
emckee2006 wants to merge 3 commits into
biojava:masterfrom
emckee2006:master