search

console-rs / indicatif

A command line progress reporting library for Rust
MIT License
4.21k stars 238 forks source link

fix and rename `real_len` #608

Closed tgolsson closed 6 months ago

tgolsson commented 7 months ago

Fixes #606 -- see added test cases. This logic matches what happens draw_target.rs as well. It'll now also correctly handle lines with only ANSI codes.

https://github.com/console-rs/indicatif/blob/81aa4c65d7202128d471b3a452a0236e03f3bc43/src/draw_target.rs#L505-L518

djc commented 6 months ago

Thanks for working on this!

Can we share the code with draw_target somehow, so that it is more obviously using the same algorithm?

If we're going to move this I'd like to see two separate commits, one that moves/renames the code and one that changes the algorithm. Ideally tests that work with the old algorithm are introduced before this, and then the fixing commit adds tests that didn't work previously -- this will make it easy for me to review. (Please squash linting changes etc.)

tgolsson commented 6 months ago

Can we share the code with draw_target somehow, so that it is more obviously using the same algorithm?

Doesn't seem like a worthwhile change seeing as that is counting lines while drawing, while this does it ahead of time.

If we're going to move this I'd like to see two separate commits, one that moves/renames the code and one that changes the algorithm. Ideally tests that work with the old algorithm are introduced before this, and then the fixing commit adds tests that didn't work previously -- this will make it easy for me to review. (Please squash linting changes etc.)

Please see new commits -- first commit moves + add working tests, second commit adds the failing tests, third commit fixes the failing tests.

tgolsson commented 6 months ago

CI seems like a flake, can't repro at all locally.