-
-
Notifications
You must be signed in to change notification settings - Fork 20
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
Use new BioGenerics macros #111
base: master
Are you sure you want to change the base?
Conversation
b18a857
to
ce53c60
Compare
@@ -14,22 +11,26 @@ PrecompileTools = "aea7be01-6a6a-4083-8856-8a6e6704d82a" | |||
StringViews = "354b36f9-a18e-4713-926e-db85100087ba" | |||
TranscodingStreams = "3bb67fe8-82b1-5028-8e26-92a6c54297fa" | |||
|
|||
[weakdeps] | |||
BioSequences = "7e6ae17a-c86d-528c-b3b9-7f778a29fe59" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh man, great use of this functionality
docs/Project.toml
Outdated
BioSequences = "7e6ae17a-c86d-528c-b3b9-7f778a29fe59" | ||
Documenter = "e30172f5-a6a5-5a46-863b-614d45cd2de4" | ||
FASTX = "c2308a5c-f048-11e8-3e8a-31650f418d12" | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need CodecZlib
here as well? Might be nice if we want doctests for compression
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good idea.
Actually, maby of the examples are not doctested because it requires a compressed file to be present. I should just add a few small dummy files for test purposes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Test dep on BioFormatSpecimens? That's what it's there for
docs/src/files.md
Outdated
A `Reader` and a `Writer` are structs that wrap an IO, and allows efficient reading/writing of FASTX `Record`s. | ||
For FASTA, use `FASTA.Reader` and `FASTA.Writer`, and for FASTQ - well I'm sure you've guessed it. | ||
For FASTA, use `FASTAReader` and `FASTAWriter`, and for FASTQ - well I'm sure you've guessed it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like these changes are duplicates from #113 . If you branched from there, just be on the lookout for merge conflicts.
docs/src/files.md
Outdated
FASTAReader(GzipDecompressorStream(open("seqs.fna.gz"; lock=false))) | ||
``` | ||
|
||
To use rdr `rdr` and `wtr` macros with `do`-syntax, use the `defer` function. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we married to defer
? Might be something useful to bikeshed. I agree we shouldn't use with
given the new use in Base
, but defer
doesn't seem intuitive from a user perspective (though I understand where it's coming from).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm completely open to suggestions. I like defer because it's short, but it's a little opaque.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In my mind, I really want to be able to use existing stuff rather than introducing a new function name. If we do need a new function name, the only thing better I can think of is apply
, but it's not much better.
The primary reason not to just make these iterables so they one can use eg map
and foreach
is the need to close the handle, right?
How does Base.eachline
handle this? It allows you to pass a file name, iterates through lines without needing an explicit open
, and doesn't need to be closed. But I guess that's only reading, not writing.
To be clear, I don't think any of this should be blocking. defer
is probably fine, and as-is this is a really big increase in usability. I don't want the perfect to be the enemy of the really good here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really want to be able to use existing stuff rather than introducing a new function name
I agree. Not sure what it should be, though.
Base.eachline
closes the handle when the iterator is exhausted. This is quite elegant, but only works for readers, not for writers. We could auto-close readers, but I'm a bit wary of guiding users towards not closing their readers, when they really do need to close their writers.
Mention the most important things first in the documentation: How to read and write files.
The new BioGenerics `@rdr_str` and `@wtr_str` macros, as well as the `defer` keyword has been a longstanding FASTX todo. This change adds this new functionality.
ce53c60
to
c403439
Compare
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #111 +/- ##
==========================================
- Coverage 89.78% 89.52% -0.26%
==========================================
Files 15 15
Lines 695 697 +2
==========================================
Hits 624 624
- Misses 71 73 +2
Flags with carried forward coverage won't be shown. Click here to find out more.
☔ View full report in Codecov by Sentry. |
The new BioGenerics
@rdr_str
and@wtr_str
macros, as well as thedefer
keyword has been a longstanding FASTX todo.This change adds this new functionality.
TODO: