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

Implemented WriteRecordExt #4

Merged
merged 6 commits into from Oct 28, 2016

Conversation

Projects
None yet
2 participants
@lannonbr
Copy link
Contributor

lannonbr commented Oct 20, 2016

I implemented the WriteRecordExt as discussed about in issue #2. With the implementation, I also added some documentation as well as a test.

@blackbeam
Copy link
Owner

blackbeam left a comment

Thanks!
But few points should be resolved before i merge.

src/lib.rs Outdated
fn write_record(&mut self, record: Record) -> Result<usize>;
}

impl WriteRecordExt for File {

This comment has been minimized.

@blackbeam

blackbeam Oct 21, 2016

Owner

I believe it should be implemented for T: io::Write which will cover every io::Write implementor including fs::File

src/lib.rs Outdated
/// write a record to a io::Write implementor
///
/// returns the length of the written record
fn write_record(&mut self, record: Record) -> Result<usize>;

This comment has been minimized.

@blackbeam

blackbeam Oct 21, 2016

Owner

Returning length here is redundant since it is using io::Write::write_all internally which is follows write all or error logic, so user always know the amount of bytes written (via record.as_ref().len()).
I think return type of WriteRecordExt::write_record should follow the return type of io::Write::write_all which is Result<()>

src/lib.rs Outdated
use super::super::*;

#[test]
fn should_write_record_to_file() {

This comment has been minimized.

@blackbeam

blackbeam Oct 21, 2016

Owner

Since WriteRecordExt will be implemented for every T: io::Write there is no need to reduce one's ssd lifetime by writing record into a file on every test run. It could be written into Vec<u8> for testing purpose.

lannonbr added some commits Oct 25, 2016

@lannonbr

This comment has been minimized.

Copy link
Contributor Author

lannonbr commented Oct 25, 2016

The changes have been made to make WriteRecordExt work for anything that implements io::Write.

src/lib.rs Outdated
impl<T> WriteRecordExt for T where T: io::Write {

fn write_record(&mut self, record: Record) -> Result<()>{
self.write_all(record.as_ref());

This comment has been minimized.

@blackbeam

blackbeam Oct 26, 2016

Owner

Result of this operation is ignored as mentioned by rustc, but it definitely shouldn't.
Also Ok(()) on line 197 is redundant since Write::write_all returns Result<(), io::Error>.
map_err(Into::into) or map_err(From::from) should solve this.

This comment has been minimized.

@lannonbr

lannonbr Oct 27, 2016

Author Contributor

I got rid of the Ok(()) and added a io::Result<()> as the return type.

src/lib.rs Outdated
Ok(_) => (),
}

assert_eq!(vec.len(), REC_SIZE as usize);

This comment has been minimized.

@blackbeam

blackbeam Oct 26, 2016

Owner

I know that implantation is straightforward, but could you assert equality of actual record content?

This comment has been minimized.

@lannonbr

lannonbr Oct 27, 2016

Author Contributor

Done.

@blackbeam blackbeam merged commit d176a0a into blackbeam:master Oct 28, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@blackbeam

This comment has been minimized.

Copy link
Owner

blackbeam commented Oct 28, 2016

Thanks!
Will publish soon.

@blackbeam

This comment has been minimized.

Copy link
Owner

blackbeam commented Oct 30, 2016

Published as v1.3.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.