feat(barchart): reduce barchart creation verbosity #936

wolfedme · 2024-02-07T14:55:18Z

BREAKING_CHANGE: Due to introduction of IntoInterator in BarGroup, the bars parameter does not accept references anymore

Wasn't able to get it fully working without breaking changes, open for suggestions if that is an issue.

Resolves #683

…troducing multiple new() methods BREAKING_CHANGE: Due to introduction of IntoInterator in BarGroup, the bars parameter does not accept references anymore

codecov · 2024-02-07T14:59:10Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (0dcdbea) 92.0% compared to head (dba4039) 92.0%.

Additional details and impacted files

@@          Coverage Diff          @@
##            main    #936   +/-   ##
=====================================
  Coverage   92.0%   92.0%           
=====================================
  Files         61      61           
  Lines      15582   15634   +52     
=====================================
+ Hits       14345   14397   +52     
  Misses      1237    1237

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

joshka

Thanks - I like these changes. It's worth taking a look to understand what the impact on apps will be for the breaking changes here. I'm concerned that if they are large, then this becomes a barrier to upgrading between versions and leads to devs being annoyed about breaking changes.

For breaking changes we like to write these up in the BREAKING-CHANGES.md doc, and in the changelog (you can use the PR body to write up a message that will appear in the changelog).

As we've just released a major version 6 days ago, I'm going to suggest that we hold off introducing merging any breaking changes too soon (this will allow us to release a point release with non-breaking changes more easily), so bear in mind that this might not be released for a month or more.

The .into() part of this is easy as we just tell users to remove the unnecessary code, but the & part seems like it would cause pain (lots of changes in apps that use charts). Perhaps write some tests of the behavior to show which parts break. You might find that this leads to a novel solution that avoids breaking this part. (Focus on unit tests over integ tests btw).

joshka · 2024-02-08T10:55:18Z

src/widgets/barchart.rs

+    pub fn new<I>(bars: I, direction: Direction) -> Self
+    where
+        I: IntoIterator<Item = Bar<'a>>,


What about making this accept into bar?

Suggested change

pub fn new<I>(bars: I, direction: Direction) -> Self

where

I: IntoIterator<Item = Bar<'a>>,

pub fn new<Iter>(bars: Iter, direction: Direction) -> Self

where

Iter: IntoIterator,

Iter::Item: Into<Bar<'a>>,

joshka · 2024-02-08T10:57:09Z

src/widgets/barchart.rs

+    ///     Direction::Vertical,
+    /// );
+    /// ```
+    pub fn new<I>(bars: I, direction: Direction) -> Self


Given the BarChart draws groups of bars, does it make sense to make this to somehow accept BarGroups instead of Bars? What might this look like if it did?

Haven't looked whether these are already there, but From<Bar> for BarGroup and From<Vec<Bar>> for BarGroup should exist then.

joshka · 2024-02-08T10:59:50Z

src/widgets/barchart/bar.rs

+    /// The ```label``` parameter accepts any type that is convertible into a [`Line`].
+    /// The ```value``` parameter is the value to be displayed on the bar.


Single backticks here, not triple.

Suggested change

/// The ```label``` parameter accepts any type that is convertible into a [`Line`].

/// The ```value``` parameter is the value to be displayed on the bar.

/// The `label` parameter accepts any type that is convertible into a [`Line`].

/// The `value` parameter is the value to be displayed on the bar.

joshka · 2024-02-08T11:01:38Z

src/widgets/barchart/bar.rs

-    /// [`Bar::value_style`] to style the value.  
+    /// [`Bar::value_style`] to style the value.
    /// [`Bar::text_value`] to set the displayed value.


These were added to put the items on separate lines, but I dislike the end of line spaces as much as your formatter does. Can you please change these to a list using - (and below)

joshka · 2024-02-08T11:02:34Z

src/widgets/barchart/bar.rs

-    pub fn label(mut self, label: Line<'a>) -> Bar<'a> {
-        self.label = Some(label);
+    pub fn label<T>(mut self, label: T) -> Bar<'a>


Add a doc comment about conversion.

Probably irrelevant with #995

joshka · 2024-02-08T11:09:09Z

src/widgets/barchart/bar.rs

I think we currently only have From impls in BarGroup, are there missing ones that might make some of this more ergonomic? E.g. From<(&'a str, u64)> for Bar perhaps?

joshka · 2024-02-08T11:11:58Z

src/widgets/barchart/bar_group.rs

@@ -21,17 +21,44 @@ pub struct BarGroup<'a> {
 }

 impl<'a> BarGroup<'a> {
+    /// Creates a new [`BarGroup`] widget with the given bars and no label.


What's the rationale for preferring no label here?

joshka · 2024-02-08T11:13:49Z

src/widgets/barchart/bar_group.rs

+    pub fn bars<I, T>(mut self, bars: I) -> BarGroup<'a>
+    where
+        I: IntoIterator<Item = T>,
+        T: Into<Bar<'a>>,


Suggested change

pub fn bars<I, T>(mut self, bars: I) -> BarGroup<'a>

where

I: IntoIterator<Item = T>,

T: Into<Bar<'a>>,

pub fn bars<Iter>(mut self, bars: Iter) -> BarGroup<'a>

where

Iter: IntoIterator

Iter::Item: Into<Bar<'a>>,

joshka · 2024-02-08T11:15:25Z

src/widgets/barchart/bar_group.rs

        self
    }

    /// Set the bars of the group to be shown
    #[must_use = "method moves the value of self and returns the modified value"]
-    pub fn bars(mut self, bars: &[Bar<'a>]) -> BarGroup<'a> {
-        self.bars = bars.to_vec();
+    pub fn bars<I, T>(mut self, bars: I) -> BarGroup<'a>


Add a note about conversion (applies generally to the new methods, won't repeat, but do a quick check for this)

Probably irrelevant with #995

wolfedme · 2024-02-14T08:52:02Z

@joshka Really appreciating the suggestions and agreeing with all of them. I am completely fine for this not being released for a month+, as it might also take a week or two until I find the time to address every suggestion :)

EdJoPaTo · 2024-04-23T17:04:17Z

src/widgets/barchart/bar.rs

+    /// [`Bar::value_style`] to style the value.
    /// [`Bar::text_value`] to set the displayed value.


Suggestion to https://github.com/ratatui-org/ratatui/pull/936/files#r1482786822

Suggested change

/// [`Bar::value_style`] to style the value.

/// [`Bar::text_value`] to set the displayed value.

/// - [`Bar::value_style`] to style the value.

/// - [`Bar::text_value`] to set the displayed value.

EdJoPaTo · 2024-04-23T17:06:14Z

src/widgets/barchart/bar_group.rs

        self
    }

    /// Set the bars of the group to be shown
    #[must_use = "method moves the value of self and returns the modified value"]
-    pub fn bars(mut self, bars: &[Bar<'a>]) -> BarGroup<'a> {
-        self.bars = bars.to_vec();
+    pub fn bars<I, T>(mut self, bars: I) -> BarGroup<'a>


Probably irrelevant with #995

EdJoPaTo · 2024-04-23T17:06:30Z

src/widgets/barchart/bar.rs

-    pub fn label(mut self, label: Line<'a>) -> Bar<'a> {
-        self.label = Some(label);
+    pub fn label<T>(mut self, label: T) -> Bar<'a>


Probably irrelevant with #995

EdJoPaTo · 2024-04-23T17:06:52Z

src/widgets/barchart/bar.rs

+    /// The ```label``` parameter accepts any type that is convertible into a [`Line`].
+    /// The ```value``` parameter is the value to be displayed on the bar.


Suggested change

/// The ```label``` parameter accepts any type that is convertible into a [`Line`].

/// The ```value``` parameter is the value to be displayed on the bar.

/// The `label` parameter accepts any type that is convertible into a [`Line`].

/// The `value` parameter is the value to be displayed on the bar.

EdJoPaTo · 2024-04-23T17:08:15Z

src/widgets/barchart.rs

+    ///     Direction::Vertical,
+    /// );
+    /// ```
+    pub fn new<I>(bars: I, direction: Direction) -> Self


Haven't looked whether these are already there, but From<Bar> for BarGroup and From<Vec<Bar>> for BarGroup should exist then.

EdJoPaTo · 2024-04-23T17:11:02Z

src/widgets/barchart/bar.rs

+        Self {
+            value,
+            label: Some(label.into()),
+            ..Default::default()


I'd prefer being explicit here.

Suggested change

..Default::default()

style: Style::new(),

value_style: Style::new(),

text_value: None,

EdJoPaTo · 2024-04-23T17:13:44Z

benches/barchart.rs

                Bar::default()
-                    .label(format!("B{i}").into())
+                    .label(format!("B{i}"))
                    .value(rng.gen_range(0..data_count))


All the examples should use the ::new function instead of default in my opinion.

EdJoPaTo · 2024-04-23T17:16:16Z

Ha, great, GitHub applies some of my comments to both answers to @joshka and as new review comments… 😒

feat(Barchart Creation): reduced verbosity of barchart creation by in…

dba4039

…troducing multiple new() methods BREAKING_CHANGE: Due to introduction of IntoInterator in BarGroup, the bars parameter does not accept references anymore

wolfedme requested review from orhun, mindoodoo, sayanarijit, joshka, kdheepak and Valentin271 as code owners February 7, 2024 14:55

joshka reviewed Feb 8, 2024

View reviewed changes

joshka added this to the v0.27.0 milestone Feb 8, 2024

EdJoPaTo changed the title ~~feat(barchart): reduced verbosity of barchart creation (#683)~~ feat(barchart): reduce barchart creation verbosity Mar 30, 2024

EdJoPaTo requested changes Apr 23, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(barchart): reduce barchart creation verbosity #936

feat(barchart): reduce barchart creation verbosity #936

wolfedme commented Feb 7, 2024

codecov bot commented Feb 7, 2024

joshka left a comment

joshka Feb 8, 2024

joshka Feb 8, 2024

EdJoPaTo Apr 23, 2024

joshka Feb 8, 2024

EdJoPaTo Apr 23, 2024

joshka Feb 8, 2024

joshka Feb 8, 2024

EdJoPaTo Apr 23, 2024

joshka Feb 8, 2024

joshka Feb 8, 2024

joshka Feb 8, 2024

joshka Feb 8, 2024

EdJoPaTo Apr 23, 2024

wolfedme commented Feb 14, 2024

EdJoPaTo Apr 23, 2024

EdJoPaTo Apr 23, 2024

EdJoPaTo Apr 23, 2024

EdJoPaTo Apr 23, 2024

EdJoPaTo Apr 23, 2024

EdJoPaTo Apr 23, 2024

EdJoPaTo Apr 23, 2024

EdJoPaTo commented Apr 23, 2024

-    pub fn new<I>(bars: I, direction: Direction) -> Self
-    where
-        I: IntoIterator<Item = Bar<'a>>,
+    pub fn new<Iter>(bars: Iter, direction: Direction) -> Self
+    where
+        Iter: IntoIterator,
+        Iter::Item: Into<Bar<'a>>,

		/// The ```label``` parameter accepts any type that is convertible into a [`Line`].
		/// The ```value``` parameter is the value to be displayed on the bar.

		/// [`Bar::value_style`] to style the value.
		/// [`Bar::text_value`] to set the displayed value.

-            ..Default::default()
+            style: Style::new(),
+            value_style: Style::new(),
+            text_value: None,

feat(barchart): reduce barchart creation verbosity #936

Are you sure you want to change the base?

feat(barchart): reduce barchart creation verbosity #936

Conversation

wolfedme commented Feb 7, 2024

codecov bot commented Feb 7, 2024

Codecov Report

joshka left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wolfedme commented Feb 14, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EdJoPaTo commented Apr 23, 2024