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

chore: updating cargo lock for toml #672

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Freyskeyd
Copy link

This PR updates toml dependency to avoid discrepancy between the latest toml version and the one in insta.

When refactoring some part of the configuration on one of my project that is using toml, I discovered that my configuration was properly serialized when using toml = 0.8.x while when using in insta it was failing withUnsupportedType. After digging a bit, it is related to this.

To reproduce on a fresh cargo new --lib :

Cargo.toml

[dependencies]
serde = { version = "1.0.210", features = ["derive"] }
toml_5 = { package = "toml", version = "0.5" }
toml_8 = { package = "toml", version = "0.8" }

lib.rs

use serde::Serialize;

#[derive(Serialize)]
enum SomeType {
    Type1 {},
    Type2 {},
}

impl Default for SomeType {
    fn default() -> Self {
        SomeType::Type1 {}
    }
}

#[derive(Serialize, Default)]
struct Config {
    #[serde(default)]
    some_type: SomeType,
}

#[cfg(test)]
mod tests {
    use super::*;

    #[test]
    #[should_panic]
    fn toml_5_test_serialize_config_fails() {
        let cfg = Config::default();

        toml_5::to_string_pretty(&cfg).unwrap();
    }

    #[test]
    fn toml_8_test_serialize_config_ok() {
        let cfg = Config::default();

        toml_8::to_string_pretty(&cfg).unwrap();
    }
}

@Freyskeyd Freyskeyd changed the title chore: updating cargo lock for ron/toml chore: updating cargo lock for toml Oct 21, 2024
@max-sixty
Copy link
Collaborator

IIUC, upgrading toml has backward compatibility concerns: #439.

Is there a version that fixes the issues you were hitting without breaking backward-compat?

@Freyskeyd
Copy link
Author

Hello, I think that 0.6 were fixing the problem, I can try.

@Freyskeyd
Copy link
Author

After trying, it is only solved on 0.8.

But I'm a little confused about this, the failing test:

━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ Snapshot Summary ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
Snapshot: toml_inline
Source: insta/tests/test_inline.rs:162
────────────────────────────────────────────────────────────────────────────────
Expression: User {
    id: 42,
    username: "peter-doe".into(),
    email: Email("[email protected]".into()),
}
────────────────────────────────────────────────────────────────────────────────
-old snapshot
+new results
────────────┬───────────────────────────────────────────────────────────────────
    0     0 │ id = 42
    1       │-username = 'peter-doe'
    2       │-email = '[email protected]'
          1 │+username = "peter-doe"
          2 │+email = "[email protected]"
────────────┴───────────────────────────────────────────────────────────────────

Should have basic string, not literal, right?
Literal would be used in place of basic string if needed, like if I use:

User {
    id: 42,
    username: "peter-doe".into(),
	display_name: r#"peter "The Machine" doe"#.into(),
    email: Email("[email protected]".into()),
}

which would produce:

id = 42
username = "peter-doe"
display_name = 'peter "The Machine" doe'
email = "[email protected]"

I understand that it can break some existing snapshot, but on the other side, it makes the library unusable for valid types. The only workaround that I found is to preserialize the struct with toml 0.8, but it prevents me to use any redactions nor filters.

@max-sixty
Copy link
Collaborator

I understand that it can break some existing snapshot, but on the other side, it makes the library unusable for valid types.

Right, but breaking existing snapshots requires us to bump the major version of insta (which is possible but there's lots of things we'd like to get in; we wouldn't do it for just this).

Is it possible to do this with a feature, like toml8? (I haven't tried this sort of thing in rust before, so not sure...)

@Freyskeyd
Copy link
Author

Is it possible to do this with a feature, like toml8? (I haven't tried this sort of thing in rust before, so not sure...)

Yes I can do that

@Freyskeyd
Copy link
Author

@max-sixty it means that we will have duplication in assert_toml_snapshot with assert_toml8_snapshot

@max-sixty
Copy link
Collaborator

Is it instead possible to do this with a feature?

@Freyskeyd
Copy link
Author

Due to the fact that we're testing with --all-features we could have both toml and toml8 feature at the same time.
Or we need to make a compile_error that defines that you can't have toml and toml8 at the same time.
Leading us to remove the all-feature flags and having to list every features that we want to enable during tests.

@max-sixty
Copy link
Collaborator

max-sixty commented Oct 23, 2024

Yes, we'd need to find some way of it using v8 when both features were enabled.

To the extent we have both versions of toml on the toml feature in your proposal — for the additional macro — are you sure we can't conditionally compile the v5 vs v8 macro as assert_toml_snapshot when the toml and not the toml8 feature is enabled?

Or if that's confusing — would be interested to see your proposal above with both dependencies active. I'd have thought the jump from that to conditionally compiling isn't that big (but may well be missing something)

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.

2 participants