Misc changes #193
Misc changes #193
Conversation
This patch simplifies the Source::collect_to() default implementation by making use of the ? operator as well as the std::iter API. Signed-off-by: Matthias Beyer <mail@beyermatthias.de>
src/value.rs
Outdated
| } | ||
|
|
||
| ValueKind::Table(r) | ||
| ValueKind::Table(values.into_iter().map(|(k, v)| (k, v.into())).collect()) |
There was a problem hiding this comment.
The new version is surely much more concise!
But on the other hand, it is harder to understand (at least for me). Maybe for the sake of readability, it could be made into two-liner like
let map = HashMap::from_iter(...);
ValueKind::Table(map)And to be sure that strings are reused drain could be used since function takes the ownership of values either way? Possibly this is what into_iter does, but I am not sure.
There was a problem hiding this comment.
Thanks for that comment. I will push a fixup commit for splitting the line into two shortly.
Using HashMap::drain is not what we want here, because it keeps the hashmap allocated for later use. HashMap::into_iter() takes ownership and with collect()ing it into a new HashMap object of HashMap<String, Value>. That's exactly what we want here, so that part is fine.
Thanks again for your comment!
src/value.rs
Outdated
| @@ -107,8 +96,15 @@ impl Display for ValueKind { | |||
| ValueKind::Nil => write!(f, "nil"), | |||
|
|
|||
| // TODO: Figure out a nice Display for these | |||
There was a problem hiding this comment.
Is this TODO still relevant?
Signed-off-by: Matthias Beyer <mail@beyermatthias.de>
Signed-off-by: Matthias Beyer <mail@beyermatthias.de>
This patch changes the Display impl for ValueKind so that Array and
Table are nicely displayed.
This basically changes a user-facing implementation in a
non-backwards-compatible way. But as the documentation for std states:
Display is similar to Debug, but Display is for user-facing output
[...]
This is user-facing and I'd say this is okay.
Signed-off-by: Matthias Beyer <mail@beyermatthias.de>
|
Because you already approved, I'm gonna squash now. |
|
Feel free to merge. |
These are misc changes that I'd like to merge. Each patch is one change that could be debated over.
I'm not gonna merge this before some discussion happened.