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

Allow pretty ser to work with implicit-some extension #182

Merged
merged 3 commits into from Aug 10, 2019

Conversation

cmaher
Copy link
Contributor

@cmaher cmaher commented Jul 13, 2019

Additionally configure newline for tests to work on windows.

I was converting my yaml files to ron and needed this, so I figured I'd submit a PR. Really enjoying RON as a data language, btw!

Additionally configure newline for tests to work on windows
Copy link
Collaborator

@kvark kvark left a comment

Thank you for the PR!

src/ser/mod.rs Outdated
@@ -112,6 +96,8 @@ pub struct PrettyConfig {
/// Enumerate array items in comments
#[serde(default = "default_enumerate_arrays")]
pub enumerate_arrays: bool,
/// Enable implicit_some extension
pub implicit_some: bool,
Copy link
Collaborator

@kvark kvark Jul 15, 2019

Choose a reason for hiding this comment

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

We would potentially want other features there as well. Let's make it a bitflags field to account for future needs?
We could move the struct_names in these flags too, sometimes later.

Copy link
Contributor Author

@cmaher cmaher Jul 16, 2019

Choose a reason for hiding this comment

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

Sure. I'm going to be busy for the next week, but I'll try to get to it after that.

@cmaher
Copy link
Contributor Author

cmaher commented Aug 4, 2019

Sorry for the delay-- I factored out the Extensions from parse, and adding it as a field on PrettyConfig (replacing the implicit_some in the initial commit). It's a bit awkward to not have it handle the struct_names, but I didn't see a clean way of handling that without breaking the existing API, which I didn't want to do here.

kvark
kvark approved these changes Aug 7, 2019
Copy link
Collaborator

@kvark kvark left a comment

This is looking great, thank you!
Just one small nit I got

src/ser/mod.rs Outdated
@@ -207,6 +208,7 @@ impl Default for PrettyConfig {
indentor: default_indentor(),
separate_tuple_members: default_separate_tuple_members(),
enumerate_arrays: default_enumerate_arrays(),
extensions: default_extensions(),
Copy link
Collaborator

@kvark kvark Aug 7, 2019

Choose a reason for hiding this comment

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

should probably just use Extensions::default() here

Copy link
Contributor Author

@cmaher cmaher Aug 10, 2019

Choose a reason for hiding this comment

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

ah, good point. will fix that right up.

@kvark
Copy link
Collaborator

kvark commented Aug 10, 2019

bors r+

bors bot added a commit that referenced this issue Aug 10, 2019
182: Allow pretty ser to work with implicit-some extension r=kvark a=cmaher

Additionally configure newline for tests to work on windows.


I was converting my yaml files to ron and needed this, so I figured I'd submit a PR.  Really enjoying RON as a data language, btw!

Co-authored-by: Christian Maher <maher.cs@gmail.com>
@bors
Copy link
Contributor

bors bot commented Aug 10, 2019

Build succeeded

@bors bors bot merged commit 29a5c11 into ron-rs:master Aug 10, 2019
2 checks passed
This was referenced Apr 30, 2020
@bpfoley bpfoley mentioned this pull request May 21, 2020
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.

None yet

2 participants