chore(widgets): Implement Debug & Default common traits
#339
Conversation
TieWay59
requested review from
orhun,
mindoodoo,
sayanarijit,
sophacles and
joshka
as code owners
Codecov Report
@@ Coverage Diff @@
## main #339 +/- ##
==========================================
- Coverage 85.08% 84.70% -0.38%
==========================================
Files 40 40
Lines 8608 8605 -3
==========================================
- Hits 7324 7289 -35
- Misses 1284 1316 +32
|
|
Hey @joshka, I've been doing this today. And I wish to share my discoveries with you before more next step of review.
|
orhun
changed the title
Nice
Makes sense, I'd even prefer to see a single commit if the split commits don't compile (as it makes it easier to
Note my comment about ordering. Of these, I'd probably say that Debug and Default are the two most important, followed by Clone and Copy. Being able to glance at a derives list and see that these are implemented is something I personally often rely on. Small nit but useful I think.
Can you point out the impact of any problems you might see with this?
Let's defer Display to another PR?
It's probably worthwhile adding default tests as it helps catch any future changes in the defaults that are accidentally made.
Perhaps these should not be implemented except for where it's necessary? Is there better guidance on this available? Are there use cases where having these makes things easier to implement that aren't obvious? |
|
@joshka Now my PR is focused on Debug & Default trait, I hope this shift can make you feel better while reviewing. And I also reordered some old patterns to align with the preferred traits order. |
|
@joshka About the coverage, Should I write a test for every struct? I found it would be very huge, and since there are still many old codes that don't have coverage before me, I'd like to hear your idea. It's OK if you feel this is necessary. But I have to warn that, since we don't have much assert_eq!(
format!("{:?}", LineGauge::default()),
format!(
"{:?}",
LineGauge {
block: None,
ratio: 0.0,
label: None,
style: Style::default(),
line_set: symbols::line::NORMAL,
gauge_style: Style::default(),
}
),
"LineGauge::default() should have correct default values."
);(Yes, it looks weird. it's my workaround, please guide me if you have a better suggestion ❤️ ) |
|
Sorry I pushed again, I found one struct I missed, and I give a shorter name for the default test. |
TieWay59
changed the title
Debug & Default common traits
|
I'd say that adding coverage on the derived traits can probably be deferred. I'll leave that up for whoever does the second review on this. |
|
I don't have a blocking review, but I was curious what the impact on library size (or binary size) this has. Is there some metrics you can add to the PR description? |
This is pretty much zero. |
Implement `Debug & Default` common traits for most structs in src.
Reorder the derive fields to be more consistent:
Debug, Default, Clone, Copy, Eq, PartialEq, Ord, PartialOrd, Hash
see: ratatui#307
|
The scrollbar::Set type moved to symbols::scrollbar::Set. I updated the commit with that change (just needs an approval to merge). Thanks for submitting this PR. It's something that I have run into a few times (particularly Debug being missing). |
|
P.s. to make future PRs easier for us merge, can you please setup commit signing? |
In #660 we introduced the segment_size field to the Table struct. However, we forgot to update the default() implementation to match the new() implementation. This meant that the default() implementation picked up SegmentSize::default() instead of SegmentSize::None. Additionally the introduction of Table::default() in an earlier PR, #339, was also missing the default for the column_spacing field (1). This commit fixes the default() implementation to match the new() implementation of these two fields by implementing the Default trait manually. BREAKING CHANGE: The default() implementation of Table now sets the column_spacing field to 1 and the segment_size field to SegmentSize::None. This will affect the rendering of a small amount of apps.
In #660 we introduced the segment_size field to the Table struct. However, we forgot to update the default() implementation to match the new() implementation. This meant that the default() implementation picked up SegmentSize::default() instead of SegmentSize::None. Additionally the introduction of Table::default() in an earlier PR, #339, was also missing the default for the column_spacing field (1). This commit fixes the default() implementation to match the new() implementation of these two fields by implementing the Default trait manually. BREAKING CHANGE: The default() implementation of Table now sets the column_spacing field to 1 and the segment_size field to SegmentSize::None. This will affect the rendering of a small amount of apps.
In #660 we introduced the segment_size field to the Table struct. However, we forgot to update the default() implementation to match the new() implementation. This meant that the default() implementation picked up SegmentSize::default() instead of SegmentSize::None. Additionally the introduction of Table::default() in an earlier PR, #339, was also missing the default for the column_spacing field (1). This commit fixes the default() implementation to match the new() implementation of these two fields by implementing the Default trait manually. BREAKING CHANGE: The default() implementation of Table now sets the column_spacing field to 1 and the segment_size field to SegmentSize::None. This will affect the rendering of a small amount of apps.
Implement
Debug & Defaultcommon traits for most structs in src.Reorder the derive fields to be more consistent:
see: #307