search

Ensembl / Bio-DB-HTS

Git repo for Bio::DB::HTS module on CPAN, providing Perl links into HTSlib
Apache License 2.0
24 stars 16 forks source link

Treat s3:// urls as remote as htslib 1.3 onwards supports it #41

Closed ah19 closed 7 years ago

ah19 commented 7 years ago

I've only added a super basic test for the method I changed rather than for the underlying s3 behaviour, I wasn't sure if you'd want a test going out to s3 as it might be flaky.

I've manually verified against htslib 1.3.2 that this does retrieve reads (via get_features_by_location) from a bam file in s3

ah19 commented 7 years ago

I've rebased on master, looks like the build is passing now

andrewyatz commented 7 years ago

Sorry you've copped for a few request changes to your test suite. Hope they're minimal changes though.

jmarshall commented 7 years ago

Is there a reason this is_remote function isn't just calling through to HTSlib's hisremote()?

rishidev commented 7 years ago

Just not realising it had been added. That might be the way to go for this.

andrewyatz commented 7 years ago

Agreed @jmarshall and @rishidev it'll be far more supportable as the response will change depending on what plugins htslib was compiled with.

@ah19 how do you fancy getting dirty with XS?

rishidev commented 7 years ago

@jmarshall Thanks for highlighting the function. @ah19 feel free to update on this PR or I can do this on a new branch (currently setting up new laptop so apologies if there's a delay in pushing it through).

ah19 commented 7 years ago

I'm off on holiday for a few weeks so I probably won't get time to, I'll just close this.

If it's still needed when I'm back I'll happily do the XS