Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

I/O API refactoring #84

Merged
merged 1 commit into from
Jun 22, 2017
Merged

I/O API refactoring #84

merged 1 commit into from
Jun 22, 2017

Conversation

totakke
Copy link
Member

@totakke totakke commented Jun 21, 2017

Breaking refactoring of I/O API.

Old cljam.io is not user-friendly, moving to cljam.io.protocols. Basically, this namespace should not be used directly on other projects. It can be used in cljam project or for advanced usage.

Public namespaces related to a format and its binary equivalent are integrated. For example,

  • cljam.io.sam supports SAM and BAM,
  • cljam.io.vcf supports VCF and BCF,
  • cljam.io.sequence supports FASTA and TwoBit.

These have read/write functions wrapping cljam.io.protocols. Only commonly-used functions are wrapped. A user just require a single namespace that handles a format.

(require '[cljam.io.sam :as sam])

(with-open [rdr (sam/reader "foo.bam")]
  (sam/read-alignments rdr))

@codecov
Copy link

codecov bot commented Jun 21, 2017

Codecov Report

Merging #84 into master will decrease coverage by 1.09%.
The diff coverage is 78.82%.

Impacted file tree graph

@@           Coverage Diff            @@
##           master     #84     +/-   ##
========================================
- Coverage      83%   81.9%   -1.1%     
========================================
  Files          63      60      -3     
  Lines        4142    4140      -2     
  Branches      432     432             
========================================
- Hits         3438    3391     -47     
- Misses        272     317     +45     
  Partials      432     432
Impacted Files Coverage Δ
src/cljam/io/fastq.clj 91.58% <ø> (ø) ⬆️
src/cljam/io/twobit/writer.clj 87.96% <ø> (ø) ⬆️
src/cljam/io/bam/decoder.clj 59.57% <ø> (ø) ⬆️
src/cljam/io/vcf/writer.clj 83.73% <ø> (ø) ⬆️
src/cljam/io/sam/reader.clj 86.2% <ø> (-10.35%) ⬇️
src/cljam/io/bam/writer.clj 72.93% <ø> (ø) ⬆️
src/cljam/io/bam/reader.clj 74% <ø> (ø) ⬆️
src/cljam/io/sam/writer.clj 70% <ø> (ø) ⬆️
src/cljam/io/sam/util.clj 87.08% <ø> (ø) ⬆️
src/cljam/io/fasta/writer.clj 93.87% <ø> (ø) ⬆️
... and 26 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7dda17a...1847c2d. Read the comment docs.

@totakke totakke requested a review from alumi June 21, 2017 07:36
@@ -136,38 +140,3 @@
#"(?i)\.bcf$" :bcf
#"(?i)\.bed" :bed
(throw (IllegalArgumentException. "Invalid file type"))))

(defn ^Closeable reader
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about leaving reader/writer functions here?
It's true that they're unnecessary for developing apps, but I think those versatile functions are still useful for REPL.

Copy link
Member Author

@totakke totakke Jun 22, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That causes cyclic load. cljam.io.sam loads cljam.io.util for cljam.io.sam/reader, and cljam.io.util loads cljam.io.sam for cljam.io.util/reader. If reader/writer are realy necessary, I have to reconsider namespace allocation.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for pointing that out. OK, Let's remove them.

@@ -136,38 +140,3 @@
#"(?i)\.bcf$" :bcf
#"(?i)\.bed" :bed
(throw (IllegalArgumentException. "Invalid file type"))))

(defn ^Closeable reader
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for pointing that out. OK, Let's remove them.

@alumi alumi merged commit 2e93bc5 into master Jun 22, 2017
@alumi alumi deleted the fix/io-refactoring branch June 22, 2017 04:18
@alumi
Copy link
Member

alumi commented Jun 22, 2017

Merged. Thank you! 👍

@totakke
Copy link
Member Author

totakke commented Jun 22, 2017

Thank you for review!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants