Fixes #3520 - respect special chars width

[shlomizadok]

No description provided.

[shlomizadok]

I honestly hate adding this ../../../ - any other way?

[ares]

doesn't just 'table_print/formatter' work? if the gem is available I'd expect it to work

[tstrachota]

I did some testing and the file formatter.rb is actually not being found at all. I'd move it from lib somewhere to 'lib/hammer_cli' and then just do

require 'hammer_cli/...'

[ares]

It would be also great to try fix it in upstream - arches/table_print#2 </unwanted tip>

[shlomizadok]

This monkey patch is breaking tests.
@mbacovsky opened a patch on the gem at arches/table_print#45
I think the only change I made was the gsub /[\u0080-\u00ff]/

Anyhow, we should try and have this on table_print and not here.

[tstrachota]

For some reason the fix doesn't work for me. It seems the file with patch is not found and the require fails silently.

[tstrachota]

I did some testing and the file formatter.rb is actually not being found at all. I'd move it from lib somewhere to 'lib/hammer_cli' and then just do

require 'hammer_cli/...'

[tstrachota]

@shlomizadok I discovered a few things while testing this PR:

The tests are failing because they don't find the file with monkey patch. When I was testing it manually it was calculating length for chinese characters ok, but it failed for czech letters like "žščř". I found out that this is primarily because of false assumption in the original table_print code. The author expects every multibyte character to be rendered in two-columns, which is not correct (https://github.com/arches/table_print/blob/master/lib/table_print/formatter.rb#L44).

So I wrote couple of tests and dug deeper into the topic. I realized it's much more complex than one would expect :) I ended up with horrible Pascal-like algorithm that calculates correct length and is able to truncate strings containing both colours and two-column characters. You can find it all here:
shlomizadok#1

Can you please review it? I'll be glad for any suggestions for making the algorithm cleaner and more readable. I wasn't able to improve it any further at the point I was writing it:)

[domcleal]

When I was testing it manually it was calculating length for chinese characters ok, but it failed for czech letters like "žščř". I found out that this is primarily because of false assumption in the original table_print code. The author expects every multibyte character to be rendered in two-columns, which is not correct

The unicode-display_width gem provides a standard way of determining the display length, I'd strongly recommend using this instead of reimplementing it:

irb(main):001:0> Unicode::DisplayWidth.of("žščř")
=> 4

[tstrachota]

Nice, it's surely better to use existing solution.

[tstrachota]

Updated. Unfortunately it can't cope with colours so some custom code still remains.

[tstrachota]

Merging that disqualifies me from doing the review :) May I ask somebody else? @mbacovsky or @ares ?

[ares]

It seems some characters still breaks it :-(

I added second name column in hammer-cli-foreman to see if it's aligned properly...

[tstrachota]

@ares you're right, that was introduced after I switched to unicode-display_width. Before that commit it works fine for czech letters. I have to add such values to the tests.

[tstrachota]

@ares updated in shlomizadok#2

[ares]

I checkout your branch and it works just fine, 👍 ACK once Shlomi merges the PR, I guess some packaging will be required once this gets in.

[mmoll]

@ares only the unicode-display_width gem needs to get packaged and hammer updated, or am I missing something? :)

[ares]

That's what I meant, I just wanted to kindly remind authors to open packaging PRs :-)

[mmoll]

what's the status here?

[tstrachota]

I found out we already have rpm package for unicode-display_width:
https://github.com/theforeman/foreman-packaging/tree/rpm/develop/rubygem-unicode-display_width

We only need debs.

[mmoll]

@tstrachota the DEB PR is there and marked ready to merge. :)

[tstrachota]

@mmoll nice! I completely missed that. Thank you!

[tstrachota]

[test]

[tstrachota]

The last thing blocking the merge is tests. @shlomizadok could you please rebase? It seems that jenkins fails to apply the patches.

[shlomizadok]

@tstrachota rebased :)

[mmoll]

💚

[tstrachota]

Merged, thanks @shlomizadok and @mmoll !