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

pretty printing regression #252

Closed
m4b opened this issue Jul 5, 2020 · 12 comments
Closed

pretty printing regression #252

m4b opened this issue Jul 5, 2020 · 12 comments
Labels

Comments

@m4b
Copy link

m4b commented Jul 5, 2020

I'm seeing a pretty printing regression in 0.6 from 0.5; looks to be in nested arrays and structs?

E.g., seeing this in diffs now:

+                ),            ],
+        )),    ],

I believe a newline is missing after the ),, since adding a newline after yields the expected pretty printed string:

                ),
            ],
        )),
    ],
@kvark kvark added the bug label Jul 7, 2020
@kvark
Copy link
Collaborator

kvark commented Jul 7, 2020

@Plecra could be related to #206 ?

@Plecra
Copy link
Contributor

Plecra commented Jul 8, 2020

Probably, yup. Someone will need to check the diff for a logic error in the tuple generator, and add a test for it.

@Plecra
Copy link
Contributor

Plecra commented Jul 8, 2020

@m4b, would you be able to provide a minimal reproduction please? I wasn't able to get it to misbehave locally.

@m4b
Copy link
Author

m4b commented Jul 9, 2020

Sure, this repro's for me:

(
    name: "foo",
    stuff: [
        Group(
            (
                name: "baz",
                group: "baz_group",
                instances: [
                    (
                        name: Some("2-2"),
                    ),
                ],
            ),
        ),
    ],
)

Repro code, if you save above as file, read it, then write it out, it should have the issue:

use serde::{Deserialize, Serialize};

use std::io::{Read, Write};

#[derive(Debug, Serialize, Deserialize, Clone)]
pub struct Foo {
    pub name: Option<String>,
}

#[derive(Debug, Serialize, Deserialize, Clone)]
pub struct StuffInner {
    pub name: String,
    pub group: String,
    pub instances: Vec<Foo>,
}

#[derive(Debug, Serialize, Deserialize, Clone)]
pub enum Stuff {
    Group(StuffInner),
}

#[derive(Debug, Serialize, Deserialize, Clone)]
pub struct Top {
    name: String,
    stuff: Vec<Stuff>,
}

fn main() {
    let args: Vec<String> = std::env::args().collect();
    let mut f = std::fs::File::open(&args[1]).unwrap();
    let r: Top = ron::de::from_reader(f).unwrap();
    let pretty = ron::ser::PrettyConfig::default();
    let ron = ron::ser::to_string_pretty(&r, pretty).unwrap();
    let mut f = std::fs::File::create(&format!("fmt.{}", &args[1])).unwrap();
    f.write_all(ron.as_bytes()).unwrap();
}

@m4b
Copy link
Author

m4b commented Jul 12, 2020

@Plecra you’re able to repro with above right ?

@Plecra
Copy link
Contributor

Plecra commented Jul 13, 2020

Nope, I actually can't. This test passes on master, just as it does on cea5d78. I'm not sure what code could've originally generated your .ron data, and I'm not seeing any problem with newlines either.

use ron::ser::{to_string_pretty, PrettyConfig};

#[test]
fn small_array() {
    use serde::{Deserialize, Serialize};

    use std::io::{Read, Write};

    #[derive(Debug, Serialize, Deserialize, Clone)]
    pub struct Foo {
        pub name: Option<String>,
    }

    #[derive(Debug, Serialize, Deserialize, Clone)]
    pub struct StuffInner {
        pub name: String,
        pub group: String,
        pub instances: Vec<Foo>,
    }

    #[derive(Debug, Serialize, Deserialize, Clone)]
    pub enum Stuff {
        Group(StuffInner),
    }

    #[derive(Debug, Serialize, Deserialize, Clone)]
    pub struct Top {
        name: String,
        stuff: Vec<Stuff>,
    }
    assert_eq!(
        to_string_pretty(&Top {
            name: "foo".into(),
            stuff: vec![
                Stuff::Group(StuffInner {
                    name: "baz".into(),
                    group: "baz_group".into(),
                    instances: vec![
                        Foo {
                            name: Some("2-2".into())
                        }
                    ]
                })
            ]
        }, PrettyConfig::new().with_new_line("\n".to_string())
    ).unwrap(),
        r#"(
    name: "foo",
    stuff: [
        Group((
            name: "baz",
            group: "baz_group",
            instances: [
                (
                    name: Some("2-2"),
                ),
            ],
        )),
    ],
)"#);
}

Edit: @kvark, I'm happy to look into this when there's a failing test for me to fix, but I won't be investigating the issue any more

@dmit
Copy link

dmit commented Jul 13, 2020

This is #240, which is fixed in master, but not in the most recent published crate version (0.6.0).

@kvark
Copy link
Collaborator

kvark commented Jul 13, 2020

Ah, that makes sense! We need another PR with the same changes but against our v0.6 branch. A separate commit would need to be added to bump the patch version and add a short changelog entry.

@torkleyy
Copy link
Contributor

Can this be closed?

@m4b
Copy link
Author

m4b commented Aug 22, 2020

The issue still exists on published 0.6 crate... can a new 0.7 be published if backporting is hard? It’s quite annoying, I have checked in ron files and the diff from this is enormous...

@kvark
Copy link
Collaborator

kvark commented Aug 23, 2020

Looks like I forgot to publish the fix. Sorry about it! v0.6.1 is now up.

@kvark kvark closed this as completed Aug 23, 2020
@m4b
Copy link
Author

m4b commented Aug 23, 2020

Thank you ! ❤️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants