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

Update dependencies #88

Merged
merged 1 commit into from
Jan 11, 2024
Merged

Conversation

daniestevez
Copy link
Contributor

This updates the dependencies to their latest version. No API changes of these updates impact confy, so no code changes are needed. The code builds and the tests run.

This closes #87.

I have not updated the crate version in Cargo.toml, but I can do that as part of this PR so that a new version can be published.

This updates the dependencies to their latest version. No API changes
of these updates impact confy, so no code changes are needed. The
code builds and the tests run.

This closes rust-cli#87.
@daniestevez daniestevez marked this pull request as draft October 20, 2023 08:17
@daniestevez
Copy link
Contributor Author

daniestevez commented Oct 20, 2023

Hold on merging. I wasn't testing with --all-features. I'll fix the problems that appear and force-push.

@daniestevez
Copy link
Contributor Author

False alarm. The features are mutually exclusive. I've now tested

cargo test && cargo test --features yaml_conf --no-default-features && cargo test --features ron_conf --no-default-features

and it passes.

@daniestevez daniestevez marked this pull request as ready for review October 20, 2023 08:20
@matthiasbeyer
Copy link
Member

Lets see what CI tells us 👍

@Zykino
Copy link
Contributor

Zykino commented Oct 20, 2023

All version change are "major" indicating a breaking change for each. Are they in our public API or not?
I mean do their change also impact us? (For example: if the directories crate changed a config folder in any OS it is breaking for us too.)

I don’t mean it to block the PR, just to have an idea of the next confy version.

@daniestevez
Copy link
Contributor Author

Fair enough. Taking a look at the changes.

directories 4 -> 5 updates dir-sys from 0.3.6 to 0.4.0 (plus an added function for Windows which doesn't seem to impact confy). dir-sys doesn't seem to have a CHANGELOG, but looking at the commit log, there doesn't seem to be anything impacting confy (the only major change is moving from winapi to windows-sys).

serde_yaml 0.8 -> 0.9. Here are the 0.9 release notes. Looking at the breaking changes, there are a couple of changes in the serialization format. I guess that for confy this means that if the configuration file does not exist and a default gets created, it can be formatted differently now. I don't know if this would be considered an API-breaking change. Another change is "A bunch of non-base-10 edge cases in number parsing have been resolved. For example 0x+1 and ++0x1 are now parsed as strings, whereas they used to be incorrectly treated as numbers.". If someone was misusing this in their configuration, this will probably break things for them.

toml 0.5 -> 0.8. Here is the CHANGELOG. The only breaking change that got my attention is "Serialization and deserialization of tuple variants has changed from being an array to being a table with the key being the variant name and the value being the array". It seems that this changes how default confy configurations are formatted, and it might also prevent older configurations from being parsed. The other changes are API changes that don't impact confy and are not in the confy public API.

@Zykino
Copy link
Contributor

Zykino commented Oct 23, 2023

Wow thanks a lot for the detailed breakdown ! 👍

I think it means (at least because of serde_yaml and toml) that we should do a breaking version too.

@deg4uss3r
Copy link
Contributor

sorry this is late!

Thanks for doing this, nice that no code changes were needed!

Before merging I would like to make sure we have some backwards compatibility between default configs, I'll look into this and get this merged and a new version (major) out.

@deg4uss3r deg4uss3r self-assigned this Dec 5, 2023
@deg4uss3r
Copy link
Contributor

Just an update, I'll find time this week -> weekend to test this. Sorry again for the delay

@deg4uss3r
Copy link
Contributor

Yup I think this is common enough we will definitely need to do a breaking version number change just based on the toml bump alone.

The following config will produce 2 different default-config.toml files that do not (de)serialize nicely:

#[derive(Debug, Serialize, Deserialize)]
enum Tup {
    O(u8, u8, u8),
}

#[derive(Debug, Serialize, Deserialize)]
struct MyConfig {
    One: u8,
    Two: String,
    Three: Tup,
}

current stable

One = 0
Two = ''
Three = [
    1,
    2,
    3,
]

This branch:

One = 0
Two = ""

[Three]
O = [
    1,
    2,
    3,
]

The old config will fail this branch with:

warning: `toml_tuple` (bin "toml_tuple") generated 3 warnings
    Finished dev [unoptimized + debuginfo] target(s) in 0.01s
     Running `target/debug/toml_tuple`
Error: BadTomlData(Error { inner: Error { inner: TomlError { message: "wanted string or table", original: Some("One = 0\nTwo = ''\nThree = [\n    1,\n    2,\n    3,\n]\n"), keys: ["Three"], span: Some(25..49) } } })

That being said, let's go ahead and get this merged. I am going to use your blurb for the serde_yaml and toml breaking changes in the README and publish a 0.6 release.

@daniestevez
Copy link
Contributor Author

Thanks for looking into this!

@deg4uss3r deg4uss3r merged commit 87e29ce into rust-cli:master Jan 11, 2024
22 checks passed
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.

Dependencies update?
4 participants