Closed correabuscar closed 3 months ago
I'll make a proof-of-concept PR that shows how it would look like if the tests supported both widths (0 and 1) because the lib already does, and without needing a build.rs
; it doesn't have to be merged, but it can be if this version of handling the issue is chosen.
Describe the bug
tl;dr: some tests still fail for users that had old
git clone
and didn'tcargo update
aftergit pull
, even though the lib works as intended. But since the lib can handle control chars of either width 0 or 1, should I mod the currently failing tests to handle both width0
or1
? and should I do it without introducing abuild.rs
? or just do nothing and thus allow tests to catch when width is!= 1
in the future ?In this commit: https://github.com/gyscos/cursive/commit/af7c3d7b5a5ce65ef90a12260a8f7c6477fa6c20
I see the intention was as you've mentioned:
in other words, you wanted it to work with either 0.1.12 or 0.1.13 (but also for tests to not fail as well), as you've mentioned here also:
however, with unicode-width 0.1.12, all control chars have width 0 (I've just tested) therefore this whole test(which already covers
\0
as well) will fail: https://github.com/gyscos/cursive/blob/332a6a4bff93f9b2d5e9540e9cc3af43e42e636e/cursive-core/src/utils/lines/spans/tests.rs#L32-L61 so not just the test for\0
that got removed in the aforementioned commit.I'm not sure(unless add
build.rs
[^1]) how to conditionally make the test depend on 0.1.12 and check for width 0, and also if >=0.1.13 check for width 1, but the initial intent was that if the assumedwidth
changes in a futureunicode-width
update, we should have tests fail, else we wouldn't know that bugs creep in due to width being different now (such as: sizes and shadows being off), even though the lib can current handle both widths: 0 or 1. Maybe given this, can mod the test forwidth
being< 2
but there's the test below which might need some tweaking too(ifbuild.rs
introduction isn't wanted).Alternatively, maybe it doesn't matter in this case that the
cargo test
will fail for users which justgit pull
-ed new changes(due to them having their ownCargo.lock
locally, because it's in.gitignore
), because, while this test fails, the lib does work as expected, and the user is in fact expected tocargo update
anyway.Additionally this test will fail too, for those users that didn't
cargo update
to get unicode-width 0.1.13:$ cargo test --example select_test -- --nocapture
, in particulartests::control_chars_become_replacement_char
andtests::nuls_become_replacement_char
.But again, all the mentioned tests only fail for users that didn't
cargo update
, not for anyone who did a freshgit clone
for example. And we kinda want these tests to fail to catch futurewidth
changes, I'd think.[^1]: without introducing a
build.rs
(which would set an expectedwidth
(based on unicode-width version, so it would be either0
or1
) in a new.rs
that we'dinclude!()
and thus can use that in the test). I'd be happy to do it, if you'd just let me know that you want this.To Reproduce
git clone
unicode-width
0.1.12cargo update
cargo test test_control_chars_have_width_1 -- --nocapture
cargo test --example select_test -- --nocapture
Expected behavior for users with stale
Cargo.lock
, tests wouldn't fail.Screenshots N/A
Environment
locale
in a terminal)Additional context nits: I forgot to mention that in commit in code comments, it's
unicode-width
instead ofunicode-segmentation
(which is at1.11.0
and has no effect on width, from what I can tell)