-
Notifications
You must be signed in to change notification settings - Fork 123
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
don't insert new lines in empty arrays or maps #150
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the PR! And extra credit for test coverage 🎖️
src/ser/mod.rs
Outdated
@@ -179,11 +181,11 @@ impl Serializer { | |||
|
|||
fn end_indent(&mut self) { | |||
if let Some((ref config, ref mut pretty)) = self.pretty { | |||
pretty.indent -= 1; | |||
if pretty.indent < config.depth_limit { | |||
self.output | |||
.extend((1..pretty.indent).map(|_| config.indentor.as_str())); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
isn't this changed now? I think you'd want to do 0..pretty.indent
if you subtract 1 earlier
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point.
Testing it out just now, I think the mechanism is insufficient to handle empty sequences and maps because deeper nested items will still have a pretty.indent > 0
I moved it around, because the test failed with extra white-space.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But this would break the non-empty sequences, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, so I've moved it back in the latest iteration, added test data to handle deeper nested empty data sets.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just didn't see the code update, hence the confusion :)
Deal with deeply nested empty maps and sets too.
c293033
to
ef02963
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good! just a few nits
src/ser/mod.rs
Outdated
@@ -163,7 +167,13 @@ impl Serializer { | |||
if let Some((ref config, ref mut pretty)) = self.pretty { | |||
pretty.indent += 1; | |||
if pretty.indent < config.depth_limit { | |||
self.output += &config.new_line; | |||
let is_empty = match self.is_empty { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could be unwrap_or(false)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, I'm still learning all the ways of Rust, unwrap_or(false)
will make that a lot simpler.
src/ser/mod.rs
Outdated
@@ -180,10 +190,19 @@ impl Serializer { | |||
fn end_indent(&mut self) { | |||
if let Some((ref config, ref mut pretty)) = self.pretty { | |||
if pretty.indent < config.depth_limit { | |||
self.output | |||
.extend((1..pretty.indent).map(|_| config.indentor.as_str())); | |||
let is_empty = match self.is_empty { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here
@torkleyy heads up in case you want to take a look |
Thank you!
Bors r+
… On Mar 5, 2019, at 23:42, Anders Rasmussen ***@***.***> wrote:
@DivineGod commented on this pull request.
In src/ser/mod.rs:
> @@ -163,7 +167,13 @@ impl Serializer {
if let Some((ref config, ref mut pretty)) = self.pretty {
pretty.indent += 1;
if pretty.indent < config.depth_limit {
- self.output += &config.new_line;
+ let is_empty = match self.is_empty {
Thanks, I'm still learning all the ways of Rust, unwrap_or(false) will make that a lot simpler.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub, or mute the thread.
|
150: don't insert new lines in empty arrays or maps r=kvark a=DivineGod This fixes #147 Co-authored-by: Anders Rasmussen <[email protected]>
Build succeeded |
This fixes #147