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

fix(table)!: ensure that default and new() match #751

Merged
merged 2 commits into from
Jan 10, 2024

Conversation

joshka
Copy link
Member

@joshka joshka commented Jan 5, 2024

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.

Copy link

codecov bot commented Jan 5, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (0614190) 92.3% compared to head (63ea4c3) 92.3%.

Additional details and impacted files
@@          Coverage Diff          @@
##            main    #751   +/-   ##
=====================================
  Coverage   92.3%   92.3%           
=====================================
  Files         57      57           
  Lines      14880   14916   +36     
=====================================
+ Hits       13746   13782   +36     
  Misses      1134    1134           

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

@joshka
Copy link
Member Author

joshka commented Jan 5, 2024

Affected repos:
https://github.com/search?q=ratatui%20AND%20%22Table%3A%3Adefault()%22%20AND%20(NOT%20org%3Aratatui-org)%20AND%20NOT%20(is%3Afork)&type=code

Note the suggested changes above.

The impact of this PR is that the column spacing will change unless you've manually specified this, and the last column will not fill the remaining space if your width constraints do not specify to do so.

Also note that segment_size is gated behind a feature flag (unstable-segment-size) if you need to use that.

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.
@joshka joshka force-pushed the fix-table-defaults branch from ff75fa9 to 5037bd3 Compare January 5, 2024 20:31
@joshka joshka changed the title fix(table): ensure that default and new() match fix(table)!: ensure that default and new() match Jan 5, 2024
@github-actions github-actions bot added the Type: Breaking Change This change will cause application code to break and must be noted in the breaking changes docs etc. label Jan 5, 2024
@Adi-df
Copy link

Adi-df commented Jan 5, 2024

Thanks for notifying the changes !

@joshka
Copy link
Member Author

joshka commented Jan 6, 2024

I'd like a second reviewer on this one before merging - mainly as a quick sanity check that this breaking change makes sense.

Copy link
Member

@Valentin271 Valentin271 left a comment

Choose a reason for hiding this comment

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

Makes sense to me, approved.

Copy link
Member

@orhun orhun left a comment

Choose a reason for hiding this comment

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

LGTM

@joshka
Copy link
Member Author

joshka commented Jan 8, 2024

Resolved conflicts in BREAKING-CHANGES.md

@orhun orhun requested a review from kdheepak January 8, 2024 11:00
@Valentin271 Valentin271 merged commit 6645d2e into main Jan 10, 2024
@Valentin271 Valentin271 deleted the fix-table-defaults branch January 10, 2024 16:16
@kdheepak
Copy link
Collaborator

It was because this PR wasn't rebased on the latest main. I thought GitHub's queue feature would catch this but that doesn't seem to be the case?

@joshka
Copy link
Member Author

joshka commented Jan 10, 2024

Squash doesn't do a rebase, CI, then merge. it just does the rebase, merge part. The changes were textually non conflicting, just semantically.

@asomers
Copy link
Contributor

asomers commented Feb 4, 2024

Thanks for notifying. But FTR, the warning for gstat-rs is a false alarm.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Breaking Change This change will cause application code to break and must be noted in the breaking changes docs etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants