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

Limit the pretty depth #93

Merged
merged 2 commits into from
Feb 9, 2018
Merged

Limit the pretty depth #93

merged 2 commits into from
Feb 9, 2018

Conversation

kvark
Copy link
Collaborator

@kvark kvark commented Feb 6, 2018

Fixes #92
cc @pyfisch

Output of cargo run --example encode:

(
    float: (2.18,-1.1,),
    tuple: ((),false,),
    map: {0:'1',8:'1',3:'5',1:'2',},
    nested: (a:"Hello from \"RON\"",b:'b',),
    var: A(255,"",),
    array: [(),(),(),],
)

Unresolved questions:

  1. should we omit the trailing comma for non-pretty output? Given that it's on the same line, it sort of makes sense. It would just affect more than just the depth_limit parameter.
  2. do we still want to put spaces after commas for pretty output behind the depth limit?

@kvark kvark requested a review from torkleyy February 6, 2018 15:44
Copy link
Contributor

@torkleyy torkleyy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice :) Please add some tests to it. As for the unresolved questions, I would answer both of them with "Yes".

@pyfisch
Copy link
Contributor

pyfisch commented Feb 7, 2018

I would answer both of them with "Yes".

Agreed. Thank you for coding this.

@kvark kvark mentioned this pull request Feb 7, 2018
@kvark
Copy link
Collaborator Author

kvark commented Feb 7, 2018

Addressing 1/2 in a separate WIP PR #94
Not sure how to efficiently test this, tbh. Any suggestions on how to proceed?

@torkleyy
Copy link
Contributor

torkleyy commented Feb 7, 2018

For the test I think asserting the above output would be enough.

@kvark
Copy link
Collaborator Author

kvark commented Feb 7, 2018

@torkleyy a test has been added. Notice that the hashmap only has one element (contrary to the encode.rs) because the ordering is not deterministic, so I don't want unrelated surprises here later down the road.

Copy link
Contributor

@torkleyy torkleyy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:

self.output += if self.separate_tuple_members() {
&config.new_line
} else {
" "
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume this gets fixed in the follow-up? Otherwise we don't have whitespaces between tuple fields.

@torkleyy
Copy link
Contributor

torkleyy commented Feb 9, 2018

bors r+

bors bot added a commit that referenced this pull request Feb 9, 2018
93: Limit the pretty depth r=torkleyy a=kvark

Fixes #92
cc @pyfisch 

Output of `cargo run --example encode`:
```yaml
(
    float: (2.18,-1.1,),
    tuple: ((),false,),
    map: {0:'1',8:'1',3:'5',1:'2',},
    nested: (a:"Hello from \"RON\"",b:'b',),
    var: A(255,"",),
    array: [(),(),(),],
)
```

Unresolved questions:
  1. should we omit the trailing comma for non-pretty output? Given that it's on the same line, it sort of makes sense. It would just affect more than just the `depth_limit` parameter.
  2. do we still want to put spaces after commas for pretty output behind the depth limit?
@bors
Copy link
Contributor

bors bot commented Feb 9, 2018

Build succeeded

@bors bors bot merged commit 4297dce into ron-rs:master Feb 9, 2018
@kvark kvark deleted the indent branch February 9, 2018 14:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Configure the depth limit for item indentation
3 participants