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

Generic Value parsing #572

Open
unicorn-madness13 opened this issue Jun 20, 2023 · 7 comments
Open

Generic Value parsing #572

unicorn-madness13 opened this issue Jun 20, 2023 · 7 comments
Labels
A-serde Area: Serde integration C-bug Category: Things not working as expected M-breaking-change Meta: Implementing or merging this will introduce a breaking change.

Comments

@unicorn-madness13
Copy link

For context, I am writing a command line application that provides configuration support via a TOML file. Much like how Git provides the config command to set config values, my app will do the same. I am wondering what the best way is to parse user input to a valid toml::Value (for example, if the user provides "true" it should be parsed to Value::Boolean(true)).

What I ended up doing is something along these lines:

fn parse(val: &str) -> toml::Value {
    let i64_val: Result<i64, _> = val.parse();
    if i64_val.is_ok() {
        return toml::Value::try_from(i64_val.unwrap()).unwrap();
    }
    
    let f64_val: Result<f64, _> = val.parse();
    if f64_val.is_ok() {
        return toml::Value::try_from(f64_val.unwrap()).unwrap();
    }
    
    let bool_val: Result<bool, _> = val.parse();
    if bool_val.is_ok() {
        return toml::Value::try_from(bool_val.unwrap()).unwrap();
    }
    
    toml::Value::try_from(val).unwrap()
}

which seems rather hacky, imo. I was hoping that this method would be available on the enum already, but it does not appear to be.

Would it be possible implement a method similar to this on the Value enum? I imagine it would return a Result instead of the actual instance, but it would be a nice convenience method.

Having skimmed the value.rs file, it looks rather easy to add but that might just be my naivety.

@epage
Copy link
Member

epage commented Jun 20, 2023

We have a impl FromStr for Value so you can do val.parse::<toml::Value>() which will parse the &str according to toml rules. Is that what you are looking for?

You might also find what cargo does interesting. Cargo allows applying configs of the format --config <KEY=VALUE>. It parses KEY=VALUE as a TOML document and ensures there is no decor (comments, newlines, etc).

See https://github.com/rust-lang/cargo/blob/768339bf2db50ad6e204ebf143b9eeac4a0dc3c4/src/cargo/util/config/mod.rs#L1280

@unicorn-madness13
Copy link
Author

That is pretty close to what I'm looking for. The one difference being that doing val.parse::<toml::Value>() errors if a key isn't provided and what I was looking for was something closer to

let val = "true";
val.parse::<toml::Value>(); // would yield toml::Value(Boolean(true))

let val = "64";
val.parse::<toml::Value>(); // would yield toml::Value(Integer(64))

let val = "foo";
val.parse::<toml::Value>(); // would yield toml::Value(String("foo"))

However, the approach that you outlined is fine. I was just wondering if I could avoid having my users type KEY=VALUE since they're not super tech savvy unfortunately.

Cargo's approach does look interesting though. If I am understanding the code correctly, it's basically loading an already defined config file and just overriding the provided values correct?

@epage
Copy link
Member

epage commented Jun 20, 2023

imo that is a bug that Value::from_str deserializes as a document, instead of a value. We intentionally split the handling of Table and Value to have distinct behavior for them.

In the mean time, you can workaround it with https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=c24e42a886c7c417c8ac9b6cfbcac250. The main problem is strings will need to be quoted.

@epage epage added C-bug Category: Things not working as expected M-breaking-change Meta: Implementing or merging this will introduce a breaking change. A-serde Area: Serde integration labels Jun 20, 2023
@unicorn-madness13
Copy link
Author

Cool beans, that will do just fine for the time being.

By the way, thank you for the quick responses! They are greatly appreciated.

@Atreyagaurav
Copy link

Is it still not fixed?

Recently had the problem.

We can do .to_string() to a Value, but can't get the Value back from that string, as the Value::from_str fails because there is no key. That seems unintuitive.

Of couse based on your example the workaround toml::Value::deserialize(toml::de::ValueDeserializer::new(&value.to_string())) seems to work well. But it's odd that, that's not the from_str for value.

@epage
Copy link
Member

epage commented Feb 7, 2024

It was waiting on a breaking release but I lost track of outstanding breaking changes.

However, the most recent breaking release is recent enough that we could likely do a new one with minimal disruption.

@epage
Copy link
Member

epage commented Feb 7, 2024

Oh, I overlooked that this was for the toml crate, not toml_edit. This doesn't have a recent breaking change to coincide this with. I'll have to think more about when to do this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-serde Area: Serde integration C-bug Category: Things not working as expected M-breaking-change Meta: Implementing or merging this will introduce a breaking change.
Projects
None yet
Development

No branches or pull requests

3 participants