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

[Merged by Bors] - default() shorthand #4071

Closed
wants to merge 3 commits into from
Closed

Conversation

cart
Copy link
Member

@cart cart commented Mar 1, 2022

Adds a default() shorthand for Default::default() ... because life is too short to constantly type Default::default().

use bevy::prelude::*;

#[derive(Default)]
struct Foo {
  bar: usize,
  baz: usize,
}

// Normally you would do this:
let foo = Foo {
  bar: 10,
  ..Default::default()
};

// But now you can do this:
let foo = Foo {
  bar: 10,
  ..default()
};

The examples have been adapted to use ..default(). I've left internal crates as-is for now because they don't pull in the bevy prelude, and the ergonomics of each case should be considered individually.

@github-actions github-actions bot added the S-Needs-Triage This issue needs to be labelled label Mar 1, 2022
@alice-i-cecile alice-i-cecile added C-Usability A targeted quality-of-life change that makes Bevy easier to use and removed S-Needs-Triage This issue needs to be labelled labels Mar 1, 2022
@alice-i-cecile
Copy link
Member

Some nits, but I like this change. It's about equally clear, shorter, and gives us a nice place to stick helpful docs.

IMO we should strongly consider applying this in bevy_ui: this is the worst crate in the ecosystem for this pattern by far IME.

@cart
Copy link
Member Author

cart commented Mar 1, 2022

IMO we should strongly consider applying this in bevy_ui: this is the worst crate in the ecosystem for this pattern by far IME.

I'm assuming you mean "uses of bevy_ui", not bevy_ui itself (which only has 3 uses of Default::default()?

@alice-i-cecile
Copy link
Member

I'm assuming you mean "uses of bevy_ui", not bevy_ui itself (which only has 3 uses of Default::default()?

Yes, in the usages. Perhaps re-export it in the bevy_ui::prelude?

@mockersf
Copy link
Member

mockersf commented Mar 1, 2022

longer term, I would like to depend less on the Default trait, as it can't be const. But until const traits are a thing, it won't be as pretty

@cart
Copy link
Member Author

cart commented Mar 1, 2022

Yes, in the usages. Perhaps re-export it in the bevy_ui::prelude?

I'm open to it, but my gut reaction is that three export locations is already more than enough (bevy::prelude::, bevy_utils::prelude::, and bevy_utils::)

longer term, I would like to depend less on the Default trait, as it can't be const. But until const traits are a thing, it won't be as pretty

Yeah as soon as a better alternative presents itself I'm open to making a switch. But const Default is also a potential future.

@alice-i-cecile
Copy link
Member

bors try

bors bot added a commit that referenced this pull request Mar 1, 2022
@ickk
Copy link
Member

ickk commented Mar 1, 2022

I'll offer my dissenting view. I don't like Default's (ab)use of the struct update syntax.

I would personally prefer something simple like

use bevy::prelude::*;

struct Foo {
  bar: usize,
  baz: usize,
}
impl Foo {
  const fn default() -> Self { Self { bar: 0, baz: 0 } }
}

let myfoo = Foo {
  bar: 10,
  ..Foo::default()
};

(not necessarily advocating for using 'default' as the name. The name of fn could be anything proto, new, w/e)

pros

  • It's more explicit about what is going on / less misleading about what 'struct update syntax' is (rather than continuing the misunderstanding that ..default() being some special language feature);
  • can be const;
  • hints to the user that they can implement/use their own struct prototype in the same way, instead of relying on the single one offered by the Default impl;
  • gets rid of the "default default" stutter.

con

  • It is more verbose to type.

@cart
Copy link
Member Author

cart commented Mar 1, 2022

It's more explicit about what is going on / less misleading about what 'struct update syntax' is (rather than continuing the misunderstanding that ..default() being some special language feature);

I dont really see the difference here. ".." is a special language feature. We are already using Rust as it is defined. Every option considered involves calling a function. The only difference is that this function is floating (and generic).

can be const

This is nice, but I consider this secondary to the goal of providing easy, ergonomic struct declarations.

hints to the user that they can implement/use their own struct prototype in the same way, instead of relying on the single one offered by the Default impl;

I'm perfectly content to let users discover this on their own time. I consider "teaching rust" a secondary goal to "using bevy".

gets rid of the "default default" stutter.

It does do that, but at the cost of significant declaration boilerplate, due to the inability of the const method to be derived, and the "stutter" of specifying the type name twice.

Copy link
Contributor

@IceSentry IceSentry left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wish rust offered cleaner options for this, but as it stands today this is the best we can do.

@ickk
Copy link
Member

ickk commented Mar 1, 2022

just to clarify my positions on some things

".." is a special language feature.

Sorry for not being clear. I meant that .. is a generally useful language feature, but so many people have a misconception that ..Default::default() is a special/magic feature that exists just for the sake of Default.

This is nice, but I consider this secondary to the goal of providing easy, ergonomic struct declarations.

I was trying to get across that personally I don't think ..default is more ergonomic in a holistic sense than ..Foo::default(). It's just slightly shorter. I think ergonomics is more than just chars typed and reducing explicitness.

It does do that, but at the cost of significant declaration boilerplate

Its not solving stutter 'at the cost of' boilerplate. ..Foo::default() doesn't introduce any additional boilerplate over ..Default::default() in order to solve the stutter; it just doesn't also solve the boilerplate problem (as this PR does), which is orthogonal to stutter, however

Not all boilerplate is bad, particularly if it aids in reasoning, comprehension, and good errors - I'd refer to #[derive(Component)], and more generally to Rust's insistence on defining types in function signatures.

Also note the (cognitive) ambiguity between ..Default::default() as a struct update, and Default::default(), as a field in a struct or Default::default() as function argument. All of these forms are likely to be seen together somewhere like ui, but can potentially mean quite different things and that's really gross imo.

example
commands.spawn_bundle(TextBundle {
  style: Style {
    margin: Rect::all(Val::Px(5.0)),
    ..Default::default()
  },
  text: Text::with_section(
    "Text Example",
    TextStyle {
      font_size: 30.0,
      font: Default::default(),
      color: Color::RED,
    },
    Default::default(),
  ),
  ..Default::default()
});

anyway, I don't think I changed any hearts today

@ickk
Copy link
Member

ickk commented Mar 1, 2022

Every option considered involves calling a function.

One last comment. There is actually another option that doesn't require a function call (but does require some namespacing) and is in some ways better, but in others worse:

#[derive(Debug)]
struct Foo {
  bar: usize,
  baz: usize,
}
impl Foo {
  const DEFAULT: Self = Self { bar: 0, baz: 0 };
}

let foo = Foo {
  bar: 10,
  ..Foo::DEFAULT
};

@cart
Copy link
Member Author

cart commented Mar 1, 2022

@ickk ok I do understand your perspective now. You make some good points, but I still think ...default() is the best solve for the constraints at hand. Thanks for the discussion!

@cart
Copy link
Member Author

cart commented Mar 1, 2022

bors r+

bors bot pushed a commit that referenced this pull request Mar 1, 2022
Adds a `default()` shorthand for `Default::default()` ... because life is too short to constantly type `Default::default()`.

```rust
use bevy::prelude::*;

#[derive(Default)]
struct Foo {
  bar: usize,
  baz: usize,
}

// Normally you would do this:
let foo = Foo {
  bar: 10,
  ..Default::default()
};

// But now you can do this:
let foo = Foo {
  bar: 10,
  ..default()
};
```

The examples have been adapted to use `..default()`. I've left internal crates as-is for now because they don't pull in the bevy prelude, and the ergonomics of each case should be considered individually.
@bors bors bot changed the title default() shorthand [Merged by Bors] - default() shorthand Mar 1, 2022
@bors bors bot closed this Mar 1, 2022
@aevyrie
Copy link
Member

aevyrie commented Mar 2, 2022

Necropost

I've made this known elsewhere, but I'm of the strong opinion that reducing character count for its own sake is antithetical to ergonomics, especially if it breaks from ecosystem conventions.

  • It's non-standard: you don't see this in other libs, which makes it a bit surprising.
  • It's "magic" - what does that function call do? How is it different from ..Default::default() or ..Foo::default()? I need to inspect the source to make sure it's not hiding any side effects.
  • It doesn't make things more ergonomic: I can already autocomplete ..Default::default() with rust-analyzer as soon as I type in ... It's possible this will now confuse RA and make autocomplete less useful, or at best RA autocomplete will now oppose Bevy's ..default() standard.

In isolation this PR isn't really a big deal, but I'd like to voice the opinion that this change is counter to its goals.

kurtkuehnert pushed a commit to kurtkuehnert/bevy that referenced this pull request Mar 6, 2022
Adds a `default()` shorthand for `Default::default()` ... because life is too short to constantly type `Default::default()`.

```rust
use bevy::prelude::*;

#[derive(Default)]
struct Foo {
  bar: usize,
  baz: usize,
}

// Normally you would do this:
let foo = Foo {
  bar: 10,
  ..Default::default()
};

// But now you can do this:
let foo = Foo {
  bar: 10,
  ..default()
};
```

The examples have been adapted to use `..default()`. I've left internal crates as-is for now because they don't pull in the bevy prelude, and the ergonomics of each case should be considered individually.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-Usability A targeted quality-of-life change that makes Bevy easier to use
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants