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

Serialize records to CSV #250

Merged
merged 1 commit into from Jul 25, 2017

Conversation

Projects
None yet
3 participants
@alisha17
Copy link
Contributor

alisha17 commented Jul 16, 2017

Fixes #232

@alisha17 alisha17 force-pushed the alisha17:serialize_csv branch from acfa52f to 5fb10dc Jul 16, 2017

@brson

This comment has been minimized.

Copy link
Contributor

brson commented Jul 19, 2017

Looks good.

Instead of doing two examples under one section, can you split it into two sections please? Call the second
perhaps "Serialize records to CSV using serde".

@brson

This comment has been minimized.

Copy link
Contributor

brson commented Jul 19, 2017

This also needs a rebase because of the merge conflict.

@alisha17 alisha17 force-pushed the alisha17:serialize_csv branch 2 times, most recently from 6616d81 to 82e1dd8 Jul 20, 2017

@budziq
Copy link
Collaborator

budziq left a comment

very nice! just some final touches


[![csv-badge]][csv] [![cat-encoding-badge]][cat-encoding]

CSV writer supports automatic serialization from Rust types into CSV

This comment has been minimized.

@budziq

budziq Jul 20, 2017

Collaborator

ease linkify the csv::Writer identifier


```rust
# #[macro_use]
#extern crate error_chain;

This comment has been minimized.

@budziq

budziq Jul 20, 2017

Collaborator

missing space after #


```rust
# #[macro_use]
#extern crate error_chain;

This comment has been minimized.

@budziq

budziq Jul 20, 2017

Collaborator

missing space after #

# }
#[derive(Debug, Serialize)]
#[serde(rename_all = "PascalCase")]

This comment has been minimized.

@budziq

budziq Jul 20, 2017

Collaborator

if the rename is important here please describe why and link to docs in the description if not then please remove it.

id: 87,
})?;
wtr.flush()?;

This comment has been minimized.

@budziq

budziq Jul 20, 2017

Collaborator

ease mention why explicit flushing is important here

let mut wtr = csv::Writer::from_writer(io::stdout());
wtr.serialize(Record {
name: "Mark",

This comment has been minimized.

@budziq

budziq Jul 20, 2017

Collaborator

how about providing a constructor for Record to make the example less verbose

}
fn run() -> Result<()> {
let mut wtr = csv::Writer::from_writer(io::stdout());

This comment has been minimized.

@budziq

budziq Jul 20, 2017

Collaborator

i would mention the key identifiers (not described in previous exame) in description linking to the docs

[![csv-badge]][csv] [![serde-badge]][serde] [![cat-encoding-badge]][cat-encoding]

The following example shows how to serialize custom structs as CSV records. For this,
make use of the [serde] crate.

This comment has been minimized.

@budziq

budziq Jul 20, 2017

Collaborator

I would remove the last sentence and just mention serde in the first sentence.

fn run() -> Result<()> {
let mut wtr = csv::Writer::from_writer(io::stdout());
wtr.write_record(&["Name", "Place", "ID"])?;

This comment has been minimized.

@budziq

budziq Jul 20, 2017

Collaborator

please mention the key identifiers in the description linking to the docs

@alisha17 alisha17 force-pushed the alisha17:serialize_csv branch from 3f6ee76 to 1515289 Jul 23, 2017

@budziq

budziq approved these changes Jul 23, 2017

Copy link
Collaborator

budziq left a comment

@alisha17 Nicely done!

Looks good to me. Unfortunately merging will have to wait for another maintainer or untill I'm back from holidays in 7days as I cannot perform final sanity checks on mobile.

@brson brson merged commit 7fa8270 into rust-lang-nursery:master Jul 25, 2017

1 check passed

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

This comment has been minimized.

Copy link
Contributor

brson commented Jul 25, 2017

Thanks @alisha17

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.