-
Notifications
You must be signed in to change notification settings - Fork 123
Implement measure_text_width with no allocation #246
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
Conversation
3d9295a
to
7329c4b
Compare
7329c4b
to
c47485b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, this mostly looks good!
src/utils.rs
Outdated
.filter(|(_, is_ansi)| !is_ansi) | ||
.map(|(sub, _)| str_width(sub)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: suggest spelling it like this:
.filter_map(|(s, is_ansi)| (!is_ansi).then(|| str_width(s)))
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I find it less readable, but that's your project 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actuallyyyy, clippy doesn't like it either! 🤓
https://rust-lang.github.io/rust-clippy/master/index.html#filter_map_bool_then
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair enough. In that case, I'd like it to be written like this (assuming clippy is okay with it):
.filter_map(|(s, is_ansi)| match is_ansi {
false => Some(str_width(s)),
true => None,
})
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Clippy is okay with that 👍
c47485b
to
0b56d9c
Compare
I'll investigate on why the tests fails later, I start to realy suspects some behavior change in unicode_width (I know this sounds cliché but it worked on my computer 😢) |
I implemented that while working on optimizing something I don't remember, it might still be useful.
83970ac
to
7a4ad64
Compare
Tests are fixed. This was just a rooky mistake of forgetting to put |
Nice, thanks! |
I implemented that while working on optimizing something I don't remember, it might still be useful.
Also, I noticed that the test for this function was wrong when only using the
unicode-width
feature (probably due to some update in the corresponding?).I've completed the Makefile so that all possible sets of feature are tested, as the project is not too big I don't think it's too heavy.