Join GitHub today
GitHub is home to over 31 million developers working together to host and review code, manage projects, and build software together.
Sign upAdd "read csv records" #320
Conversation
budziq
requested changes
Oct 7, 2017
|
Nice! @ludwigpacifici Only sone minor stylistic suggestions and we are ready to merge! |
| } | ||
| #[derive(Debug, Deserialize)] | ||
| struct Record { |
This comment has been minimized.
This comment has been minimized.
budziq
Oct 7, 2017
Collaborator
I would move the struct definition to the beginning of the snippet. this will improve the reading flow somewhat
|
|
||
| [![csv-badge]][csv] [![cat-encoding-badge]][cat-encoding] | ||
|
|
||
| Reads standard CSV records into [`csv::StringRecord`] (valid UTF-8 bytes). |
This comment has been minimized.
This comment has been minimized.
budziq
Oct 7, 2017
Collaborator
(valid UTF-8 bytes).
the meaning of this last part is unclear. Could you elaborate the description?
This comment has been minimized.
This comment has been minimized.
ludwigpacifici
Oct 8, 2017
Author
Contributor
From the documentation: https://docs.rs/csv/1.0.0-beta.4/csv/struct.StringRecord.html
A string record permits reading or writing CSV rows that are valid UTF-8. [...] If you do need to read possibly invalid UTF-8 data, then you should prefer using a ByteRecord, since it makes no assumptions about UTF-8.
Is it part of the cookbook to paraphrase the rust/crates documentation? I think it is not a good idea to duplicate it, and actually I prefer to remove:
(valid UTF-8 bytes).
The reader can follow the link for more precise and up to date documentation.
This comment has been minimized.
This comment has been minimized.
budziq
Oct 8, 2017
Collaborator
Hmm it might be quite surprising to the users that their valid csv will not work with this example due to uncommon encoding. As this is the most basic csv example it should be the most informative and ordered as first (we will reorder the deserialization examples later on). I would actually suggest to mentioning this distinction between StringRecord and ByteRecord.
This comment has been minimized.
This comment has been minimized.
ludwigpacifici
Oct 8, 2017
Author
Contributor
Make sense. Should it be mentioned in the description and in an additional snippet?
This comment has been minimized.
This comment has been minimized.
budziq
Oct 8, 2017
Collaborator
the end of the main description should be enough but I'll leave it up to you to put it where it will make mist sense
This comment has been minimized.
This comment has been minimized.
ludwigpacifici
Oct 8, 2017
Author
Contributor
I will mention it in the description. However, the extra snippet is not useful. In the first example code, it's only a matter of replacing from for record in reader.records() { to for record in reader.byte_records() {
This comment has been minimized.
This comment has been minimized.
budziq
Oct 8, 2017
Collaborator
I wouldn't say that the second snippet is not useful. But handling non UTF-8 data there would be more involved so lets leave it as its is.
| for record in reader.deserialize() { | ||
| let record: Record = record?; | ||
| println!("{:?}", record); |
This comment has been minimized.
This comment has been minimized.
budziq
Oct 7, 2017
Collaborator
how about we emphasize the strongly typed nature of the deserialization by also using one of the fields directly?
This comment has been minimized.
This comment has been minimized.
| let mut reader = csv::Reader::from_reader(csv.as_bytes()); | ||
| for record in reader.records() { | ||
| println!("{:?}", record?); |
This comment has been minimized.
This comment has been minimized.
budziq
Oct 7, 2017
Collaborator
how about we emphesize the dynamic nature of this variant by using the record fields by its index?
This comment has been minimized.
This comment has been minimized.
budziq
merged commit 8c80e34
into
rust-lang-nursery:master
Oct 8, 2017
This comment has been minimized.
This comment has been minimized.
|
@ludwigpacifici Nicely done! |
This comment has been minimized.
This comment has been minimized.
|
It's a pleasure! Thanks for your reviews @budziq |
ludwigpacifici commentedOct 7, 2017
•
edited by budziq
fixes #228