Remove Default when useless on its own

[EdJoPaTo]

Description

Some structs implement Default. When using said default its pointless on its own. It requires other methods which can easily lead to mistakes.

What is a Terminal::default()? (Chart)Dataset::default() without data.

To Reproduce

Just using default without something else is pointless:

let datasets = vec![
    Dataset::default(),
    Dataset::default(),
];
let x_axis = Axis::default();
let y_axis = Axis::default();
let chart = Chart::new(datasets)
    .x_axis(x_axis)
    .y_axis(y_axis);

// lets render... nothing?
let chart = Chart::default();

Expected behavior

Default should work on its own without further modifications.
Rust is well suited to create only valid data structures. The builder pattern is a great example for this preventing invalid data structures on build().

There should be a new method accepting the required data instead of manipulating a default.

-Dataset::default().data(data);
+Dataset::new(data);

In this case the data method should also be removed to have a single way of creating the struct. Especially with the single-use, short-lived structs there is no point in keeping the setters rather than confusion.

When structs contain other structs that have a Default implementation which does not make any sense on its own then Option::None is a way better fit instead of some pointless default.

Migration suggestion:

• create new method with required arguments, deprecate the setter methods which are required, pointing towards the new method.
• after some time remove said setters and Default implementation.

Defaults which seem useless

An easy indicator: When the documentation of the struct never uses the default() on its own.

Some are debatable like Style (its very convenient to use Style::default() over None) and especially widgets are straight forward.

• BarChart
  • BarGroup
  • Bar
• Block (default is useless but with titles and borders probably the simplest way to keep it that way?)
  • Title
• Buffer
  • Cell
• CalendarEventStore (unsure)
• Canvas
  • Label
  • Layer
  • BrailleGrid
  • CharGrid
  • HalfBlockGrid
  • Circle
  • Line
  • Map (drawing with Color::Reset me be intended but very unlikely)
  • Rectangle
• Chart
  • Axis
  • ChartLayout
  • Dataset
• Clear (Clear::default() -> Clear)
• Constraint (Either you don't need a layout or you want something specific?)
• Gauge
  • LineGauge
• Layout (Either you want a specific layout or you don't need it)
  • Margin (default is likely a bug? Explicit zero or something calculated)
  • Size (default is likely a bug? Explicit zero or something calculated)
• Line (default is likely a bug? Should be explicitly empty?)
  • Span
  • Text
• List
• Masked (why mask nothing?)
• Paragraph
  • Wrap
• Scrollbar
  • ScrollbarState
  • symbols::scrollbar::Set (many empty str, what a benefit)
• Sparkline
• Style (unstyled == Option<Style>::None?)
  • Modifier (why created in the first place then?)
• StyledGrapheme (default is likely a bug? Should always be something?)
• Table
  • Cell
  • Row
• Tabs
• Terminal
  • CrosstermBackend
    • ModifierDiff
  • TermionBackend
    • Fg
    • Bg
    • ModifierDiff
• reflow
  • WordWrapper
  • LineTruncator

[Valentin271]

I partly agree with that, some points to consider.

1. Removing Default is a breaking change (which I'd be down to push), and thus requires proper deprecation

2. I don't think your example is good when we consider widgets could be stored (especially when rendering by ref). The setter is useful to avoid the entire re-creation of the widget.

I think we should keep the existing setters to avoid an unnecessary breaking change. I'd also be very careful about which Default to remove, to me an empty Dataset makes sense. But then it could be created more explicitly, which could be better.

[EdJoPaTo]

1. Removing Default is a breaking change (which I'd be down to push), and thus requires proper deprecation

Yes, but we can not mark them as deprecated directly. That's why I think the only way to do it is with a bigger breaking release or by marking the setters deprecated. Which is also not ideal.

2. I don't think your example is good when we consider widgets could be stored (especially when rendering by ref). The setter is useful to avoid the entire re-creation of the widget.

I think there are two approaches to widgets: Storing the state or being thinner wrappers taking references. Currently the second one is used. rendering by ref would go towards storing stuff inside the widget itself. This question should be answered before thinking about removing the setters.

Personally I would like to always have useable things. If they should be empty (like your Dataset example), they should be created explicitly. It should not be possible to do so by accident. The API should push the people to use it correctly. Having a single way to use an object is the best way in doing so.

widgets could be stored

With serde support that is indeed something we need to think about.

[joshka]

First up - I'm definitely in agreement that values should be generally constructed in a usable state and the API should push the construction of valid structs.

There are a few things I think are worth considering to this:

• partial construction of types is something that the builder approach makes possible, so initially useless values is often an intentionally allowed state.
• Existing code should be generally easy to update to use the new methods
• Use github searches to understand the impact of the change on downstream code - generally I use search queries like ratatui "barchart::default()" AND (NOT org:ratatui-org) AND (NOT is:fork)
• Doing too many breaking changes at once causes user angst

[EdJoPaTo]

• partial construction of types is something that the builder approach makes possible, so initially useless values is often an intentionally allowed state.

Normally with the builder pattern there are two structs, the target one which only exists when possible and the builder which is kind of default in the beginning. Ratatui has both merged into a single struct.

• Doing too many breaking changes at once causes user angst

I think removing Default should be done in its own update with a clear path what to do (default() -> new(something)). Before that we should update all the examples and so on, so we know this path is working well. Also, users looking at examples will already use that way and don't consider using default() as they haven't seen it somewhere. Marking required setters as deprecated pointing towards the new() method will also ease the path before the breaking change comes. I think that will reduce a lot of pain points on the breaking change. And we notice early what does not work well so far and can adapt before the breaking change happens.

[joshka]

Normally with the builder pattern there are two structs, the target one which only exists when possible and the builder which is kind of default in the beginning. Ratatui has both merged into a single struct.

Yeah - I think this was probably a mistake when compared to more formal builder approaches, but it's what we inherited, and it worked well for many apps. The pattern is sometimes called the builder-lite pattern. https://matklad.github.io/2022/05/29/builder-lite.html (and the associated reddit thread has more discussion)

I think removing Default should be done in its own update with a clear path...

Agreed.

I wonder whether it's worth considering a more explicit builder approach for the incremental build needs? Effectively meaning T::default().x() becomes T::builder().x().build() in user code.

[EdJoPaTo]

I wonder whether it's worth considering a more explicit builder approach for the incremental build needs? Effectively meaning T::default().x() becomes T::builder().x().build() in user code.

For most widgets I don't think so. When a List has ListItems (even an empty Vec) its valid. The rest is optional. Keeping them that way is just simpler I guess.

So its only relevant for things like a Block which on its own is useless but has border or title which both make it useful.

[joshka]

https://rust-lang.github.io/api-guidelines/interoperability.html#c-common-traits

Note that it is common and expected for types to implement both Default and an empty new constructor. new is the constructor convention in Rust, and users expect it to exist, so if it is reasonable for the basic constructor to take no arguments, then it should, even if it is functionally identical to default.

[joshka]

There are a lot of structs mentioned in this. If you do decide to work on this, please don't submit a PR that is large and does all of these. This type of PR tends to be difficult to review reasonably and languishes in the PR queue because of that. More useful would be a smaller targeted PR for a few of these at a time.

However, I think the result of this seems to be that we should not remove default when it's reasonable and only remove it where it's patently obvious that there is no sensible default. Does that align with your thinking on this @EdJoPaTo?