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

Whitespace change after editing when updating from 0.22.20 to 0.22.21 #787

Closed
torokati44 opened this issue Sep 23, 2024 · 10 comments · Fixed by #788
Closed

Whitespace change after editing when updating from 0.22.20 to 0.22.21 #787

torokati44 opened this issue Sep 23, 2024 · 10 comments · Fixed by #788

Comments

@torokati44
Copy link

torokati44 commented Sep 23, 2024

In our regular weekly dependency bump (ruffle-rs/ruffle#18048), we have a failing test, here:
https://github.com/ruffle-rs/ruffle/blob/0a9d2bae1e9abab279fe8d20e13b2ee8e19a5bc2/frontend-utils/src/bookmarks/write.rs#L118-L142

The failure is:

thread 'bookmarks::write::tests::overwrite_invalid_bookmark_type' panicked at frontend-utils/src/bookmarks/write.rs:67:5:
assertion `left == right` failed
  left: "[[bookmark]]\nurl = \"file:///test.swf\"\nname = \"test.swf\"\n"
 right: "[[bookmark ]]\nurl = \"file:///test.swf\"\nname = \"test.swf\"\n"

Could you please advise on whether this is on us to fix, it's inconsequential because it doesn't matter for the toml syntax, or perhaps a bug in toml_edit?

@epage
Copy link
Member

epage commented Sep 23, 2024

Nothing in the changelog indicates any intentional whitespace changes.

Can you create an independent reproduction case?

@torokati44
Copy link
Author

torokati44 commented Sep 23, 2024

Sure, here you go:

use toml_edit::{DocumentMut, Table};

fn main() {
    let mut document = "bookmark = 1010".parse::<DocumentMut>().unwrap();

    let key: &str = "bookmark";

    document.insert(key, toml_edit::array());
    let table = document[key].as_array_of_tables_mut().unwrap();

    let mut bookmark_table = Table::new();
    bookmark_table["name"] = toml_edit::value("test.swf".to_string());
    table.push(bookmark_table);

    let expected: &str = "[[bookmark]]\nname = \"test.swf\"\n";
    assert_eq!(expected, document.to_string());
}

This passes with 0.22.20, but fails with 0.22.21:

thread 'main' panicked at src/main.rs:16:5:
assertion `left == right` failed
  left: "[[bookmark]]\nname = \"test.swf\"\n"
 right: "[[bookmark ]]\nname = \"test.swf\"\n"

@epage
Copy link
Member

epage commented Sep 24, 2024

It appears the root cause was 189697d.

@epage
Copy link
Member

epage commented Sep 24, 2024

The exact change that caused this is at 189697d#r147105791

This is an interesting case. It looks like this might actually be a bug fix as we are preserving the user's original formatting in more cases. In this case its a problem for the user because they are changing from a key-value pair to a table where the decor no longer applies.

@epage
Copy link
Member

epage commented Sep 24, 2024

insert_formatted is not overriding formatting, like it is supposed to.

In light of insert_formatted, I'm tempted to keep insert not preserving the existing formatting.

@torokati44
Copy link
Author

Just so I'm not mistaken: Does this mean that plain insert will resume working like it used to, in a patch release; or that we're supposed to switch to insert_formatted, or do something else...?

@epage
Copy link
Member

epage commented Sep 24, 2024

For now, I'm restoring 0.22.20's behavior, see f74124d

@torokati44
Copy link
Author

Yeah, just saw that, thank you!

"For now" - and how about later?

@epage
Copy link
Member

epage commented Sep 24, 2024

"For now" - and how about later?

Likely based on feedback and, at this point, through a breaking change

@torokati44
Copy link
Author

Thank you for the quick fix! ^^

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 a pull request may close this issue.

2 participants