Handle invalid CSV data with Serde #237

alisha17 · Jul 11, 2017

fixes #231

budziq · Jul 11, 2017

budziq requested changes Jul 11, 2017

View changes

@alisha17 almost perfect!
Just few nits below:

please also update the master TOC in intro.md
please check why the travis tests are failing
please update the description with fixes #231 so the issue autocloses with this PR

budziq · Jul 11, 2017

src/encoding.md

+    id: u64,
+}
+
+fn run() -> Result<(), Box<Error>> {


please avoid using Box use the error_chain instead

budziq · Jul 11, 2017

src/encoding.md

+    Ok(())
+}
+
+fn main() {


please use quick_main from error-chain crate

budziq · Jul 11, 2017

src/encoding.md

@@ -8,6 +8,7 @@
 | [Encode a string as application/x-www-form-urlencoded][ex-urlencoded] | [![url-badge]][url] | [![cat-encoding-badge]][cat-encoding] |
 | [Encode and decode hex][ex-hex-encode-decode] | [![data-encoding-badge]][data-encoding] | [![cat-encoding-badge]][cat-encoding] |
 | [Encode and decode base64][ex-base64] | [![base64-badge]][base64] | [![cat-encoding-badge]][cat-encoding] |
+| [Handle invalid CSV data with Serde][ex-invalid-csv] | [![csv-badge]][csv] | [![cat-encoding-badge]][cat-encoding] |


please also add the serde badge

budziq · Jul 11, 2017

src/encoding.md

+<a name="ex-invalid-csv"></a>
+## Handle invalid CSV data with Serde
+
+[![csv-badge]][csv] [![cat-encoding-badge]][cat-encoding]


please add serde badge

budziq · Jul 11, 2017

src/encoding.md

+#[serde(rename_all = "PascalCase")]
+struct Record {
+    Name: String,
+    #[serde(deserialize_with = "csv::invalid_option")]


this serde macro is important in avoiding deserialization errors. pleas mention it in description and add links to docs.

budziq · Jul 11, 2017

src/encoding.md

+use std::process;
+
+#[derive(Debug, Deserialize)]
+#[serde(rename_all = "PascalCase")]


If this serde macro is important then please mention it in description and add link to docs

budziq · Jul 11, 2017

src/encoding.md

+[![csv-badge]][csv] [![cat-encoding-badge]][cat-encoding]
+
+We get deserialization errors if invalid CSV data like empty field
+is encountered. Serde converts any such errors to a None value, when applied


if mentioning identifiers, please linkify them to docs

budziq · Jul 11, 2017

src/encoding.md

+    let data = "\
+        Name,Place,id
+        Mark,,46
+        Ashley,Zurich,92";


please add a linebreak here

budziq · Jul 11, 2017

src/encoding.md

+}
+
+fn run() -> Result<(), Box<Error>> {
+    let data = "\


do we need this linebreak here?

budziq · Jul 11, 2017

src/encoding.md

+fn run() -> Result<(), Box<Error>> {
+    let data = "\
+        Name,Place,id
+        Mark,,46


this field is actually valid csv the place will be an empty string
please see the https://docs.rs/csv/1.0.0-beta.3/csv/fn.invalid_option.html for suggestion on invalid data from serde point of view.

budziq · Jul 15, 2017

Hi @alisha17 thanks for all the submissions!

Unfortunately I'll not be able to follow up with anything resembling review for the next two weeks due to holidays 😞 . I may drop some comments when Im on mobile later and I've asked other maintainers to drop from time to time and look into the PR's.

brson · Jul 19, 2017

brson reviewed Jul 19, 2017

View changes

brson · Jul 19, 2017

src/encoding.md

+
+[![csv-badge]][csv] [![serde-badge]][serde] [![cat-encoding-badge]][cat-encoding]
+
+We get deserialization errors if [`invalid CSV data`] is encountered. [`Serde`] is


Please remove the backticks from "invalid CSV data" - backticks are for formatting text as code and this is just regular text. Same with "Serde".

brson · Jul 19, 2017

brson reviewed Jul 19, 2017

View changes

brson · Jul 19, 2017

src/encoding.md

+
+We get deserialization errors if [`invalid CSV data`] is encountered. [`Serde`] is
+essential to convert any such errors to a [`None`] value, when applied to
+the [`Option`] field.


The second sentence here, "Serde is essential to convert any errors ..." seems off to me. This examples looks to be about using csv::invalid_option to deal with bogus CSV data.

I might instead word this as something like

CSV files often contain invalid data. For these cases the csv crate provides a custom deserializer, csv::invalid_option, which automatically converts invalid data to None values.

Place a link on csv::invalid_option, but remove the links from Some and None - those are very fundamental types that don't need links here IMO.

brson · Jul 19, 2017

I added some further feedback about the description of this test. This needs a rebase.

budziq · Jul 20, 2017

budziq requested changes Jul 20, 2017

View changes

Nice! just few more details and we are golden.

budziq · Jul 20, 2017

src/encoding.md

+provides a custom deserializer, [`csv::invalid_option`], which automatically
+converts invalid data to None values.
+
+[`csv::invalid_option`]: https://docs.rs/csv/*/csv/fn.invalid_option.html


please move this link to the bottom of the file under referencje section

budziq · Jul 20, 2017

src/encoding.md

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


missing space after #

budziq · Jul 20, 2017

src/encoding.md

+
+fn run() -> Result<()> {
+    let data = "name,place,id
+                mark,\"\",46


the value here is actually valid ( string containing """").
please note that in the example from docs has a i32 type trying to be deserialized both from such string and xyz which are both invalid i32.

another approach would be to actually leave out one of the fields (remove the , too).

If we leave out one field, it will actually give error of fields: "found record with 2 fields, but the previous record has 3 fields".

Hmm the docs seem of then. I'll lok into it after holidays.

ok then exchange the Optional field to non string one as almost anything you throw at it will produce a string.

budziq · Jul 20, 2017

src/encoding.md

+fn run() -> Result<()> {
+    let data = "name,place,id
+                mark,\"\",46
+                ashley,zurich,92";


I would add two more records. one valid and another invalid in another way.

budziq · Jul 20, 2017

src/encoding.md

+    name: String,
+    #[serde(deserialize_with = "csv::invalid_option")]
+    place: Option<String>,
+    id: u64,


how about making all of the fields optional as in the docs for csv::invalid_option?

budziq · Jul 23, 2017

budziq requested changes Jul 23, 2017

View changes

one final thing and we are good to go.

budziq · Jul 23, 2017

src/encoding.md

+
+#[derive(Debug, Deserialize)]
+struct Record {
+    name: Option<String>,


do the Option's here make sense without csv::invalid_option? these might obfuscate the example.

I think it actually doesn't really matter. But yes, in the example it's better to keep Option with invalid field only. I will fix it.

yep the code is still valid but newb reader might get confused when seeing more Options than the relevant one. 👍

budziq · Jul 24, 2017

budziq approved these changes Jul 24, 2017

View changes

@alisha17 Nicely done!

As previously merging will have to wait a little

brson · Jul 25, 2017

I rebased and merged manually. Thanks @alisha17

alisha17 force-pushed the alisha17:invalid_data branch from 8af57ab to ca6f3bd Jul 11, 2017

alisha17 force-pushed the alisha17:invalid_data branch from 24136f3 to c1f5086 Jul 15, 2017

alisha17 force-pushed the alisha17:invalid_data branch 2 times, most recently from 56bf92d to bffa320 Jul 20, 2017

alisha17 force-pushed the alisha17:invalid_data branch from cd0db61 to 6d81608 Jul 23, 2017

Handle invalid CSV data with serde

Loading status checks…

4fa3a38

alisha17 force-pushed the alisha17:invalid_data branch from e43448d to 4fa3a38 Jul 24, 2017

brson closed this Jul 25, 2017

rust-lang-nursery/rust-cookbook

Join GitHub today

Handle invalid CSV data with Serde #237

Conversation

alisha17 commented Jul 11, 2017 • edited

alisha17 force-pushed the alisha17:invalid_data branch from 8af57ab to ca6f3bd Jul 11, 2017

budziq requested changes Jul 11, 2017 View changes

budziq left a comment • edited

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

alisha17 force-pushed the alisha17:invalid_data branch from 24136f3 to c1f5086 Jul 15, 2017

This comment has been minimized.

budziq commented Jul 15, 2017

brson reviewed Jul 19, 2017 View changes

This comment has been minimized.

brson reviewed Jul 19, 2017 View changes

This comment has been minimized.

This comment has been minimized.

brson commented Jul 19, 2017

alisha17 force-pushed the alisha17:invalid_data branch 2 times, most recently from 56bf92d to bffa320 Jul 20, 2017

budziq requested changes Jul 20, 2017 View changes

budziq left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

alisha17 force-pushed the alisha17:invalid_data branch from cd0db61 to 6d81608 Jul 23, 2017

budziq requested changes Jul 23, 2017 View changes

budziq left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

alisha17 force-pushed the alisha17:invalid_data branch from e43448d to 4fa3a38 Jul 24, 2017

budziq approved these changes Jul 24, 2017 View changes

budziq left a comment

This comment has been minimized.

brson commented Jul 25, 2017

brson closed this Jul 25, 2017

alisha17 commented Jul 11, 2017 •

edited

alisha17 force-pushed the alisha17:invalid_data branch from `8af57ab` to `ca6f3bd` Jul 11, 2017

budziq requested changes Jul 11, 2017

View changes

budziq left a comment •

edited

alisha17 force-pushed the alisha17:invalid_data branch from `24136f3` to `c1f5086` Jul 15, 2017

brson reviewed Jul 19, 2017

View changes

brson reviewed Jul 19, 2017

View changes

alisha17 force-pushed the alisha17:invalid_data branch 2 times, most recently from `56bf92d` to `bffa320` Jul 20, 2017

budziq requested changes Jul 20, 2017

View changes

alisha17 force-pushed the alisha17:invalid_data branch from `cd0db61` to `6d81608` Jul 23, 2017

budziq requested changes Jul 23, 2017

View changes

alisha17 force-pushed the alisha17:invalid_data branch from `e43448d` to `4fa3a38` Jul 24, 2017

budziq approved these changes Jul 24, 2017

View changes