Skip to content

feat(width): add is_ok() success indicator to Wrap and Truncate (#186)#576

Open
mvanhorn wants to merge 1 commit into
zhiburt:masterfrom
mvanhorn:osc/186-wrap-truncate-success
Open

feat(width): add is_ok() success indicator to Wrap and Truncate (#186)#576
mvanhorn wants to merge 1 commit into
zhiburt:masterfrom
mvanhorn:osc/186-wrap-truncate-success

Conversation

@mvanhorn

Copy link
Copy Markdown
Contributor

Summary

Adds a way for callers to detect whether Width::wrap or Width::truncate actually had to do anything, without manually checking grid.total_width after the fact.

Closes #186

Why this matters

Issue #186 noted that today users have to compare grid.total_width before and after to know whether wrapping/truncation actually happened. The maintainer's suggestion in the issue body was to add a flag to the struct and impl TableOption on &mut, exposing the result via something like wrap.is_ok(). This PR implements that pattern for both Wrap and Truncate.

let mut wrap = Width::wrap(80);
table.with(&mut wrap);
if wrap.is_ok() {
    // table actually got wrapped
}

Changes

  • tabled/src/settings/width/wrap.rs: added success: bool field to Wrap, is_ok() accessor, and a new TableOption impl for &mut Wrap<W, P> that mirrors the existing owned impl.
  • tabled/src/settings/width/truncate.rs: same shape for Truncate.
  • tabled/tests/settings/width_test.rs: 6 tests covering the work-needed path, the already-fits path, and the no-op edge case where the request is below the table's minimum (borders/padding floor) and no cell content actually changes.

The existing owned-value TableOption impls and call sites are untouched. The new &mut impls are purely additive.

Self-review notes

Two things from a self-review pass that aren't obvious from the diff:

  • Clone bounds on the new impls. The new &mut impls clone self.width.clone() and self.priority.clone() so they can construct a one-shot owned Wrap / Truncate to feed into the existing wrap_total_width / truncate_total_width helpers. The Measurement and Peaker traits are already Clone for the standard implementations (PriorityNone, PriorityMax, PriorityMin), so this didn't tighten the bound for any in-tree call site.

  • is_ok() only reflects real work, not "below total width". A naive implementation could set success = true whenever the requested width is below the rendered total, but that misclassifies the case where the table's minimum is determined by borders/padding rather than cell content -- in which case no cell text changes. The implementation now compares column widths before and after wrap_total_width / truncate_total_width and only marks success when they actually differ. The two *_below_minimum_borders_padding tests assert this with a Builder::from_iter([[""]]).build().with(Style::modern()) table and Width::wrap(1) / Width::truncate(1) -- the rendered output is unchanged and is_ok() correctly returns false.

Testing

$ cargo build --workspace
ok

$ cargo test --package tabled --test main -- wrap truncate
running 64 tests
... (all pass) ...
test result: ok. 64 passed; 0 failed; 0 ignored

$ cargo test --package tabled --test main -- wrap_is_ok truncate_is_ok
running 6 tests
test settings::width_test::wrap_is_ok_true_when_wrap_needed                              ... ok
test settings::width_test::wrap_is_ok_false_when_table_already_fits                      ... ok
test settings::width_test::wrap_is_ok_false_when_request_below_minimum_borders_padding   ... ok
test settings::width_test::truncate_is_ok_true_when_truncate_needed                      ... ok
test settings::width_test::truncate_is_ok_false_when_table_already_fits                  ... ok
test settings::width_test::truncate_is_ok_false_when_request_below_minimum_borders_padding ... ok

$ cargo fmt --check
clean

This contribution was developed with AI assistance (Codex).

…urt#186)

Closes zhiburt#186

Adds a way for callers to detect whether `Width::wrap` or
`Width::truncate` actually had to do anything, without manually
checking `grid.total_width` after the fact.

The new pattern follows the maintainer's hint in the issue:

  let mut wrap = Width::wrap(80);
  table.with(&mut wrap);
  assert!(wrap.is_ok());  // true if wrapping was actually required

- Add a `success: bool` field to both `Wrap` and `Truncate`
  (default false, set true on the wrapping/truncating branch).
- Add an `is_ok()` accessor on both structs.
- Add a new `TableOption` impl for `&mut Wrap<W,P>` and
  `&mut Truncate<'_, W, P>` that mirrors the existing owned impl
  and writes `self.success = true` when actual work occurs.
- 4 new tests in `tabled/tests/settings/width_test.rs` cover both
  the work-needed and already-fits paths for each struct.

Existing owned-value impls and call sites are untouched -- the new
`&mut` impls are purely additive, so no existing code path changes
behavior.
@zhiburt

zhiburt commented Apr 25, 2026

Copy link
Copy Markdown
Owner

Hi @mvanhorn

I'll read it through in the morning.

But this idea:

let mut wrap = Width::wrap(80);
table.with(&mut wrap);
if wrap.is_ok() {
    // table actually got wrapped
}

Is very very very very valuable.
The only question is in implementation.


There's more options which can fail and produce no actual change to the table.
For example:
All settings with .modify() if index is wrong no change will be made, or BorderCorrection.
Yes.... probably this is not that valuable as for width/height.

Just wonder if there's more and if we could produce some interface for it.
Or adding this logic to all of them.

Also what about name is_ok, is_success, success?


Just saying things out loud (a little bit exited about this feature).

Maybe a decorator like the next.
We could compare config/data before and after and based on this say was that a successful operation or not.
*It's possible but maybe less beautiful

let mut result = tabled::SettingsResult::new(Width::wrap(80));
table.with(&mut result);
if result.is_ok() {
}

Once again great proposal @mvanhorn will check it tomorrow.

@mvanhorn

Copy link
Copy Markdown
Contributor Author

Glad it resonates. No rush on the read-through, and happy to iterate on the implementation shape if you have a different signature in mind once you've had time to look.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add an indication of success to Wrap/Truncate

2 participants