jrobinso commented 5 years ago

This PR adds a URLHelper factory interface and function on ParsingUtils to set the factory. It is a replacement or alternative to the previous implementation which used reflection to accomplish the same thing. Reflection won't work for Java 11 clients, its illegal across module boundaries for the most part, and bad practice as well. The reflection API was left in place in case anyone is using it with Java 8.

codecov-io commented 5 years ago

Codecov Report

Merging #1421 into master will decrease coverage by 0.004%. The diff coverage is 27.273%.

@@               Coverage Diff               @@
##              master     #1421       +/-   ##
===============================================
- Coverage     68.122%   68.118%   -0.004%     
+ Complexity      8382      8381        -1     
===============================================
  Files            573       573               
  Lines          33989     33988        -1     
  Branches        5668      5669        +1     
===============================================
- Hits           23154     23152        -2     
- Misses          8645      8646        +1     
  Partials        2190      2190

Impacted Files	Coverage Δ	Complexity Δ
...main/java/htsjdk/tribble/util/RemoteURLHelper.java	`46.154% <ø> (ø)`	`3 <0> (ø)`	:arrow_down:
src/main/java/htsjdk/tribble/util/FTPHelper.java	`0% <ø> (ø)`	`0 <0> (ø)`	:arrow_down:
src/main/java/htsjdk/tribble/util/HTTPHelper.java	`47.917% <0%> (-3.194%)`	`4 <0> (ø)`
...rc/main/java/htsjdk/tribble/util/ParsingUtils.java	`74.863% <37.5%> (+1.179%)`	`44 <1> (-1)`	:arrow_down:
...samtools/util/AsyncBlockCompressedInputStream.java	`72% <0%> (-4%)`	`12% <0%> (-1%)`
src/main/java/htsjdk/variant/vcf/VCFHeader.java	`90.116% <0%> (+0.058%)`	`76% <0%> (ø)`	:arrow_down:
...n/java/htsjdk/variant/vcf/VCFContigHeaderLine.java	`80.556% <0%> (+4.085%)`	`11% <0%> (+1%)`	:arrow_up:

lbergelson commented 5 years ago

@jrobinso Is reflection really not working for java 11? I was under the impression that java 11 was giving a nasty warning but allowing it and it would be disabled at some time in the future.

jrobinso commented 5 years ago

Not between modules.

On Fri, Oct 4, 2019 at 12:41 PM Louis Bergelson notifications@github.com wrote:

@jrobinso https://github.com/jrobinso Is reflection really not working for java 11? I was under the impression that java 11 was giving a nasty warning but allowing it and it would be disabled at some time in the future.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/samtools/htsjdk/pull/1421?email_source=notifications&email_token=AAHD2HDVBGUJCWDZFILLJX3QM6L75A5CNFSM4I45F7VKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEAMWHPQ#issuecomment-538534846, or mute the thread https://github.com/notifications/unsubscribe-auth/AAHD2HHNFS2GFLNV7LD6WVLQM6L75ANCNFSM4I45F7VA .

-- Sent from Gmail Mobile

jrobinso commented 5 years ago

@lbergelson Thanks for the quick review and comments! I'll take care these early next week, but first would you prefer a breaking change that removes the old reflection stuff? As you say its ugly and not forward compatible with Java 11 and on, so maybe we should just get rid of it. Ditto removing or making private that public static variable. Let me know.

lbergelson commented 5 years ago

I vote for removing the old reflection code and that public static variable.

jrobinso commented 5 years ago

@lbergelson OK I'll give that a try and update the PR.

jrobinso commented 5 years ago

@lbergelson working on this now, but slightly off-topic I stumbled on a deprecation warming for the method htsjdk.tribble.util.URLHelper> openInputStreamForRange. This method is used extensively by IGV for tribble-indexed files. The IGV implementation is essentially the same as the htsjdk, with no reports of bugs traced to it, so I think its stable.


    /**
     * May throw an OperationUnsupportedException
     * @deprecated Will be removed in a future release, as is somewhat fragile
     * and not used.
     * @param start
     * @param end
     * @return
     * @throws IOException
     */
    @Deprecated
    InputStream openInputStreamForRange(long start, long end) throws IOException;

lbergelson commented 5 years ago

@jrobinso That was deprecated in 2013 by Jacob Siltera, so I figured that IGV didn't need it. We can undeprecate it if it's useful and well used.

jrobinso commented 5 years ago

@lbergelson updated, have a look I'm using this version in IGV now (in my dev environment)

samtools / htsjdk

Add a URLHelper factory API to ParsingUtils #1421

Codecov Report