Envious uses env insertion order not _n_ values to determine order of a Vec

[arifd]

Hi, first off, fantastic crate!!

use serde::Deserialize;

#[derive(Deserialize, Debug)]
struct Config {
    order: Vec<String>,
}

fn main() {
    std::env::set_var("order__1_", "second");
    std::env::set_var("order__0_", "first");
    let config: Config = envious::Config::default().build_from_env().expect("Could not deserialize from env");
    assert_eq!(config.order, vec!["first".to_string(), "second".to_string()]);
}

This will panic, because second was declared first, despite giving it a numeric value of 1.

Not sure how you feel, but I found this suprising UX.

Many thanks!

[TheNeikos]

Heya! 👋 Happy you like the crate.

This will panic, because second was declared first, despite giving it a numeric value of 1.

Sadly that is one of the shortcomings currently. I remember it being a bit complicated in serde, because structs can also be encoded as arrays. And I did not have the time yet to fix it.

PRs welcome if anyone wants to take a shot at it!

[arifd]

Great stuff, thanks for the prompt response! Note that this could be a huge breaking change for anyone who was previously relying on the env declaration order instead of the numeric index.

I think possibly for this reason also, rejecting anything other than numeric index might be a good idea... since it's kind of meaningless and surprising otherwise?

[tenuous-guidance]

I'm not the repo owner, so take my responses with a grain of salt.

rejecting anything other than numeric index might be a good idea

I could see somebody doing a, b, etc. Maybe even a_1, a_2, b_1, b_2 to split by categories too (e.g. grouping all enum variants together in a Vec<MyEnum>).

this could be a huge breaking change

I've tried to write a decent CHANGELOG to go along with the version bump that indicates a breaking change, but I can submit a new MR if you think it's unclear?

relying on the env declaration order

I think it would be odd, but it is configurable to support env declaration order if somebody really wants it. However I've not defaulted to the old behaviour, even though the new default makes it a breaking change as imo that's almost never the behaviour somebody would want.

[arifd]

Hmmm... I'm honestly not sure what the best thing to do is. :/

[TheNeikos]

Breaking changes are fine. Going from 0.2 -> 0.3 is not a 'simple' version bump. Cargo will not automatically update to it.

I don't think that dependabot ignores semver here.

Since I do have to require that people know what semver is that is the best I can do here.