Pretty configuration options #61

kvark · 2017-10-11T18:40:26Z

Fixes #58
~~BREAKING CHANGE~~

kvark · 2017-10-11T18:44:49Z

Non-separated tuple members:

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

Separated tuple members:

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

torkleyy

Nice, I love these options. Also solves the question about which line endings to use. Found a few nits, see below.

torkleyy · 2017-10-11T18:54:16Z

src/ser/mod.rs

+    fn default() -> Self {
+        PrettyConfig {
+            #[cfg(not(target_os = "windows"))]
+            new_line: "\n".to_string(),


torkleyy · 2017-10-11T18:56:31Z

src/ser/mod.rs

    struct_names: bool,
 }

 impl Serializer {
+    fn separate_tuple_members(&self) -> bool {
+        match self.pretty {


Nit: could use map and unwrap_or

I don't think it would look much better

It's just the style that seems to be preferred. You can also leave it as is if you prefer this style.

torkleyy · 2017-10-11T18:58:06Z

src/ser/pretty.rs

-use serde::ser::Serialize;
-
-/// Serializes `value` in the recommended RON layout.
-pub fn to_string<T>(value: &T) -> Result<String>


I don't see why we have to make this a breaking change.

Could we just add a second variant and leave this function, using PrettyConfig::default?

Not sure if it's worth it? All it takes from the user is to pass in default() as a parameter

although, you are right, by keeping the old function we can make it a non-breaking change

Right, I just don't see why we should bump the version.

torkleyy · 2017-10-11T19:03:26Z

src/ser/mod.rs

+    /// New line string
+    pub new_line: String,
+    /// Indentation string
+    pub indentor: String,


Could theoretically use &str or &[u8] instead.

I tried, facing the lifetime hell since the &'a (assuming we don't want &'static) starts polluting Serializer struct

Yeah, let's leave it like that then. Pretty serialization isn't something that suffers from two string allocations I guess.

torkleyy · 2017-10-11T19:04:36Z

src/ser/mod.rs

 struct Pretty {
    indent: usize,
 }

+/// Pretty serializer configuration
+pub struct PrettyConfig {


Let's derive Copy, Clone, Debug and serde traits for this struct.

kvark · 2017-10-11T19:17:35Z

Thanks for the review, please take another look!

torkleyy · 2017-10-11T19:20:05Z

src/ser/mod.rs

 use std::fmt::{Display, Formatter, Result as FmtResult};

 use serde::ser::{self, Serialize};

+#[deprecated]


I wouldn't deprecate it. Better just move the new function to that module. If you really want to, we should give an explanation.

I don't see why we need pretty module, it's empty and serves no solid purpose.

And what deprecated tells you is basically that it's going to be cut off in the next breaking release, which is what I wanted to do in the first place :)

Yes, but let's add an explanation that one should simply use to_string_pretty with a default config instead.

So #[deprecated(since="0.1.4", note="please use to_string_pretty with PrettyConfig::default() instead")]

kvark · 2017-10-11T20:16:37Z

@torkleyy agreed. How about now?

torkleyy

Thanks! After you've addressed that last thing, feel free to merge.

torkleyy · 2017-10-11T20:19:22Z

src/ser/mod.rs

 struct Pretty {
    indent: usize,
 }

+/// Pretty serializer configuration
+#[derive(Clone, Debug)]


We need Deserialize and Serialize, too.

kvark · 2017-10-11T20:31:06Z

Unfortunately, that would require serde_derive dependency in non-test configuration

…

On Oct 11, 2017, at 16:20, Thomas Schaller ***@***.***> wrote: @torkleyy approved this pull request. Thanks! After you've addressed that last thing, feel free to merge. In src/ser/mod.rs: > struct Pretty { indent: usize, } +/// Pretty serializer configuration +#[derive(Clone, Debug)] We need Deserialize and Serialize, too. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or mute the thread.

torkleyy · 2017-10-11T20:43:57Z

So what's the problem?

kvark · 2017-10-11T20:56:30Z

@torkleyy well, one more dependency imposed for all the users. Not sure if it's worth it, considering how marginal the need to serialize the config is.

Aceeri · 2017-10-11T21:52:11Z

@kvark serde re-exports serde_derive if you use the "derive" feature flag I believe.

kvark · 2017-10-11T23:20:29Z

This can be done later at any point, if anyone requests the feature.

…

On Oct 11, 2017, at 17:52, Aceeri ***@***.***> wrote: @kvark serde re-exports serde_derive if you use the "derive" feature flag I believe. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

kvark · 2017-10-12T13:00:27Z

@torkleyy added serde_derive unconditionally now

torkleyy

Thank you for addressing all my concerns!

torkleyy · 2017-10-12T15:46:14Z

Let's, finally, merge this :)

bors r+

61: Pretty configuration options r=torkleyy a=kvark Fixes #58 ~~BREAKING CHANGE~~

bors · 2017-10-12T15:48:22Z

Build succeeded

continuous-integration/travis-ci/push

kvark requested a review from torkleyy October 11, 2017 18:40

torkleyy requested changes Oct 11, 2017

View reviewed changes

kvark force-pushed the pretty branch from 2087c7f to d8dacf6 Compare October 11, 2017 19:16

torkleyy reviewed Oct 11, 2017

View reviewed changes

kvark force-pushed the pretty branch from d8dacf6 to e9388cf Compare October 11, 2017 20:15

torkleyy approved these changes Oct 11, 2017

View reviewed changes

kvark added 2 commits October 12, 2017 09:00

Pretty configuration

d601290

Using serde_derive unconditionally

ed79046

kvark force-pushed the pretty branch from b0e69a0 to ed79046 Compare October 12, 2017 13:00

torkleyy approved these changes Oct 12, 2017

View reviewed changes

bors bot added a commit that referenced this pull request Oct 12, 2017

Merge #61

2fd7050

61: Pretty configuration options r=torkleyy a=kvark Fixes #58 ~~BREAKING CHANGE~~

bors bot merged commit ed79046 into master Oct 12, 2017

kvark deleted the pretty branch October 12, 2017 17:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pretty configuration options #61

Pretty configuration options #61

kvark commented Oct 11, 2017 •

edited

Loading

kvark commented Oct 11, 2017

torkleyy left a comment

torkleyy Oct 11, 2017

torkleyy Oct 11, 2017

kvark Oct 11, 2017

torkleyy Oct 11, 2017

torkleyy Oct 11, 2017

torkleyy Oct 11, 2017

kvark Oct 11, 2017

kvark Oct 11, 2017

torkleyy Oct 11, 2017

torkleyy Oct 11, 2017

kvark Oct 11, 2017

torkleyy Oct 11, 2017

torkleyy Oct 11, 2017

kvark commented Oct 11, 2017

torkleyy Oct 11, 2017

kvark Oct 11, 2017

kvark Oct 11, 2017

torkleyy Oct 11, 2017

torkleyy Oct 11, 2017

kvark commented Oct 11, 2017

torkleyy left a comment

torkleyy Oct 11, 2017

kvark commented Oct 11, 2017 via email

torkleyy commented Oct 11, 2017

kvark commented Oct 11, 2017

Aceeri commented Oct 11, 2017

kvark commented Oct 11, 2017 via email

kvark commented Oct 12, 2017

torkleyy left a comment

torkleyy commented Oct 12, 2017

bors bot commented Oct 12, 2017

Pretty configuration options #61

Pretty configuration options #61

Conversation

kvark commented Oct 11, 2017 • edited Loading

kvark commented Oct 11, 2017

torkleyy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kvark commented Oct 11, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kvark commented Oct 11, 2017

torkleyy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kvark commented Oct 11, 2017 via email

torkleyy commented Oct 11, 2017

kvark commented Oct 11, 2017

Aceeri commented Oct 11, 2017

kvark commented Oct 11, 2017 via email

kvark commented Oct 12, 2017

torkleyy left a comment

Choose a reason for hiding this comment

torkleyy commented Oct 12, 2017

bors bot commented Oct 12, 2017

Build succeeded

kvark commented Oct 11, 2017 •

edited

Loading