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

Ergonomics of optional fields #55

Closed
ozkriff opened this issue Oct 2, 2017 · 15 comments
Closed

Ergonomics of optional fields #55

ozkriff opened this issue Oct 2, 2017 · 15 comments

Comments

@ozkriff
Copy link

ozkriff commented Oct 2, 2017

If I have a struct like

#[derive(Debug, Clone, Deserialize)]
pub struct Settings {
    pub font: Option<PathBuf>, // <- optional field
    pub other_value: u8,
}

I can omit the field in .ron file to get None:

(
    other_value: 0,
)

But I'm forced to write Some() around the actual value which clutters up the config:

(
    font: Some("OpenSans-Regular.ttf"), // :(
    // font: "OpenSans-Regular.ttf", // I want this
    other_value: 0,
)

Seems like an ergonomic problem to me :(

See https://gitter.im/ron-rs/ron?at=59d228bd614889d47565733d

@torkleyy
Copy link
Contributor

torkleyy commented Oct 2, 2017

@ozkriff If you don't specify a font, you'll use some fallback, right?

@ozkriff
Copy link
Author

ozkriff commented Oct 2, 2017

Correct.

@torkleyy
Copy link
Contributor

torkleyy commented Oct 2, 2017

#[derive(Deserialize)]
struct Settings {
    #[serde(default = "default_font")]
    font: String,
}

fn default_font() -> String {
    "default.ttf".to_owned()
}

@ozkriff
Copy link
Author

ozkriff commented Oct 3, 2017

The problem in this specific case is that if there's no font in the config, I want to use embedded font:

let font = if let Some(ref fontpath) = settings.font {
    new_font_from_vec(fs::load(fontpath))
} else {
    let data = include_bytes!("Karla-Regular.ttf");
    new_font_from_vec(data.to_vec())
};

@kvark
Copy link
Collaborator

kvark commented Oct 3, 2017

@ozkriff isn't it easy to workaround? set default to something like "<embedded>", then check for it and load your font correspondingly

@ozkriff
Copy link
Author

ozkriff commented Oct 3, 2017

@kvark yep, that's what I'm going to do. Though it's still a hack and I think I would prefer some kind of "(configurable) option promotion" from RON or something.

@torkleyy
Copy link
Contributor

torkleyy commented Oct 3, 2017

@kvark Do we want an option for implicit Some? And if so, should it be on by default?

@kvark
Copy link
Collaborator

kvark commented Oct 3, 2017

@torkleyy I don't see a strong for it yet. The serde defaults should cover it mostly, and help keep ron simpler and more consistent.

@oraknabo
Copy link

I understand how it makes sense to work around it from the Rust side of things, but it does feel really counter-intuitive to have to wrap lots of values with Some() on the scripting side.

Even though the scripter should know whether a field is optional or not, I feel like it expects too much understanding of Rust. I've done something similar to with TOML in a previous project and it handled options implicitly. I don't mean to imply that you should do it just because they did, but it just felt more like the right way to do things from the scripting side of things.

Could the parser check to see if an option is expected and wrap the value with Some() automatically?

@torkleyy
Copy link
Contributor

The current strategy was chosen because it is more concise. E.g. None could be None in Rust or Some(None).

@kvark
Copy link
Collaborator

kvark commented Jan 13, 2018 via email

@torkleyy
Copy link
Contributor

I already started working on a solution, but couldn't finish it yet.

@oraknabo
Copy link

oraknabo commented Jan 13, 2018

@kvark @torkleyy It just feels like the default behavior is a little inconsistent:

  • I have optional_field: Option<String> in Rust.
  • If I omit the field in a .ron file, it accepts it as None, no "expected option" error message.
  • If I supply a String, I get an "expected option" error.

Shouldn't it see that I correctly supplied a String and accept it (same as with None) since String and None are both acceptable values for optional_field?

@torkleyy
Copy link
Contributor

You can only skip the field if you added #[serde(skip)] .

@Boscop
Copy link

Boscop commented Jan 15, 2018

If missing fields become None, and existing fields become Some(val), that would mean the format won't be self-describing any longer, right?
I don't have a problem with that though, as it's intended to be written by humans..
But how to distinguish Some(None) from None etc? The same way as serde_json?

bors bot added a commit that referenced this issue Jan 17, 2018
75: Allow implicit Some r=kvark a=torkleyy

Fixes #55
@bors bors bot closed this as completed in #75 Jan 17, 2018
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

No branches or pull requests

5 participants