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

write_* takes ownership of Writer #8

Closed
d-cameron opened this issue Oct 25, 2021 · 3 comments
Closed

write_* takes ownership of Writer #8

d-cameron opened this issue Oct 25, 2021 · 3 comments

Comments

@d-cameron
Copy link

d-cameron commented Oct 25, 2021

Maybe I'm confused about the API but why do seq_io::fast*::write_* take ownership of the writer? Doesn't this mean you can only ever write one record? This seems problematic.

The simplest reproduction example is:

let mut reader = Reader::from_path("in.fastq")?;
let mut writer = BufWriter::new(File::open(Path::new("out.fastq"))?);
for r in reader.records() {
    let r = r?;
    seq_io::fastq::write_to(writer, &r.head, &r.seq, &r.qual);
}

which doesn't compile since writer was moved.

@d-cameron
Copy link
Author

The function signature is:

pub fn write_to<W: io::Write>(
    mut writer: W,

Shouldn't it be?

pub fn write_to<W: std::io::Write>(
    writer: &mut W,

@markschl
Copy link
Owner

markschl commented Oct 25, 2021

I'm sorry if writing isn't documented well. I'm working on a next version with better docs, but it is progressing slowly.

The following should work, because io::Write is also implemented for mutable references to Write instances:

seq_io::fastq::write_to(&mut writer, &r.head, &r.seq, &r.qual)?;

Like this, the writer is not moved to the writing function. Also note the ?, since it is necessary to handle write errors.

It is even simpler to use the write method of records (it's a bit hidden because it's in the Record trait:

r.write(&mut writer)?;

Did you use the records() method on purpose for iterating over records? It will yield OwnedRecord instances, which is comparable to the Rust-Bio reading methods. In case reading performance is important for you and you don't need to store the record objects for later use, I would advise to use the while let Some(r) = reader.next() construct. It returns a RefRecord borrowing its data from the underlying reader, and consequently no vector allocations and (mostly) no copying are required. See the bottom of the GitHub page for how the two compare (borrowed on top, owned below).

Your code would then be:

let mut reader = Reader::from_path("in.fastq")?;
let mut writer = BufWriter::new(File::create("out.fastq")?);
wile let Some(r) = reader.next() {
    let r = r?;
    r.write(&mut writer)?;
}

Also note that you need to use File::create instead of File::open to open a file in write mode, and that constructing a Path instance is not necessary because File::create accepts AsRef<Path> which &str implements.

@d-cameron
Copy link
Author

d-cameron commented Oct 25, 2021

The following should work, because io::Write is also implemented for mutable references to Write instances:

Aah, it's the magic in the Write trait itself that I missed. Add the expected &mut and the magic happens.

Thanks for the explanation :)

Also note that you need to use File::create instead of File::open to open a file in write mode

That one got picked up in my testing pretty quickly.

Did you use the records() method on purpose for iterating over records?

Mostly it was because the borrowed version didn't appear to implement Iter and I was switching from another fastq parsing library.

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

No branches or pull requests

2 participants