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

Properties 2.0 - Required unless annotated with default value #928

Closed
jstarry opened this issue Feb 8, 2020 · 9 comments · Fixed by #975
Closed

Properties 2.0 - Required unless annotated with default value #928

jstarry opened this issue Feb 8, 2020 · 9 comments · Fixed by #975
Labels

Comments

@jstarry
Copy link
Member

jstarry commented Feb 8, 2020

Properties 2.0

Problem

Properties without #[props(required)] or #[props(default = "fn_name")] are implicitly set to their default value.

Proposal

Properties should be treated as required unless indicated otherwise

New syntax:

#[derive(Clone, Properties)]
struct Props {
  #[prop_or(3)],
  countdown: usize,

  #[prop_or_else(Callback::noop)]
  on_click: Callback<()>,

  #[prop_or(true)]
  display: bool,

  #[prop_or_default]
  highlight: bool,

  // implicitly required
  required: MyRequiredValue,

  #[prop_or_default]
  opt_value: Option<Value>,

  // implicitly required
  opt_required: Option<Value>,
}

Old syntax:

#[derive(Clone, Properties)]
struct Props {
  #[props(default = "countdown_default")],
  countdown: usize,

  #[props(default = "Callback::noop")]
  on_click: Callback<()>,

  #[props(default = "display_default")]
  display: bool,

  // implicitly optional and will default to `false`
  highlight: bool,

  #[props(required)]
  required: MyRequiredValue,

  // implicitly optional and will default to `None`
  opt_value: Option<Value>,

  #[props(required)]
  opt_required: Option<Value>,
}

fn countdown_default() -> usize {
  3
}

fn display_default() -> bool {
  true
}

Explanation

Now that we have a default attribute for properties, I think it makes sense to require it for indicating when properties are optional. So this means we would drop support for #[props(required)]

You may ask, why not follow the precedent that serde set for default values? Link

Serde was designed before arbitrary token streams were allowed for helper attributes in Rust 1.34.0: https://blog.rust-lang.org/2019/04/11/Rust-1.34.0.html#custom-attributes-accept-arbitrary-token-streams (kudos to @jplatte for pointing this out)

I propose changing the derive props macro attributes from props to prop_or_else, prop_or_default. Reference: https://doc.rust-lang.org/reference/procedural-macros.html#derive-macro-helper-attributes I feel that or, or_else and or_default naming is more Rust-y and the or_else helps inform developers that the default expression for a prop will not be run at runtime unless a prop is not present.

@jstarry jstarry added the feature-request A feature request label Feb 8, 2020
@jplatte
Copy link
Contributor

jplatte commented Feb 8, 2020

It's supposed to be default = …, not default: …, right?

@jstarry
Copy link
Member Author

jstarry commented Feb 9, 2020

@jplatte I believe either is supported by Rust now. I thought default: ... looked nicer but default = ... is fine too

@jplatte
Copy link
Contributor

jplatte commented Feb 9, 2020

Yeah, either should be possible. I've just never seen the colon syntax used in attributes. AFAIK all builtin attributes use = (e.g. #[path = "foo.rs"], #[cfg(feature = "feat")] ).

@jstarry jstarry changed the title Add default attribute to the derive macro for Properties [derive-props] Treat properties as required unless annotated with a default value Feb 16, 2020
@jstarry jstarry added proposal and removed feature-request A feature request labels Feb 16, 2020
@jstarry jstarry changed the title [derive-props] Treat properties as required unless annotated with a default value Properties 2.0 - Required unless annotated with default value Feb 16, 2020
@jstarry jstarry changed the title Properties 2.0 - Required unless annotated with default value Props 2.0 - Required unless annotated with default value Feb 16, 2020
@jstarry jstarry changed the title Props 2.0 - Required unless annotated with default value Properties 2.0 - Required unless annotated with default value Feb 16, 2020
@jstarry
Copy link
Member Author

jstarry commented Feb 16, 2020

@jplatte @mdtusz @AlephAlpha @hgzimmerman I just updated this issue to be a new proposal for how we handle props and would like input. This builds upon the work that @AlephAlpha already did in #881

@jplatte
Copy link
Contributor

jplatte commented Feb 19, 2020

I like the new proposal :)

We could maaaybe have #[prop_or_else(Callback::noop)] (fn) and #[prop_or(true)] (value) to mirror Options / Results API, but I don't know if that really makes things simpler.

@jstarry
Copy link
Member Author

jstarry commented Feb 19, 2020

Good call, that would probably be more in line with expected behavior. It'd be closer to Option's API

@mdtusz
Copy link
Contributor

mdtusz commented Feb 19, 2020

No comments from me. Looks like a great improvement for usability and grok-ability!

@AlephAlpha
Copy link
Contributor

I'm trying to implement this. Should we support the old syntax for backward compatibility?

@jstarry
Copy link
Member Author

jstarry commented Feb 29, 2020

No need, would complicate things too much

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants