Fixes #3520 - respect special chars width by shlomizadok · Pull Request #218 · theforeman/hammer-cli

shlomizadok · 2016-09-27T13:29:33Z

No description provided.

shlomizadok · 2016-09-27T13:30:02Z

lib/hammer_cli/output/adapter/table.rb

@@ -1,5 +1,6 @@
 require 'table_print'
 require File.join(File.dirname(__FILE__), 'wrapper_formatter')
+require File.join(File.dirname(__FILE__), '../../../table_print/formatter')


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

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

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 · 2016-09-27T13:36:19Z

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

shlomizadok · 2016-09-28T12:51:00Z

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 · 2016-10-04T15:07:19Z

lib/hammer_cli/output/adapter/table.rb

@@ -1,5 +1,6 @@
 require 'table_print'
 require File.join(File.dirname(__FILE__), 'wrapper_formatter')
+require File.join(File.dirname(__FILE__), '../../../table_print/formatter')


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 · 2016-11-14T12:24:30Z

@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:
https://github.com/shlomizadok/hammer-cli/pull/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 · 2016-11-14T12:30:51Z

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 · 2016-11-14T12:50:48Z

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

tstrachota · 2016-11-14T13:06:31Z

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

tstrachota · 2016-11-15T14:06:31Z

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

ares · 2016-11-15T16:02:40Z

It seems some characters still breaks it :-(

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

tstrachota · 2016-11-16T10:26:39Z

@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 · 2016-11-16T10:42:43Z

@ares updated in https://github.com/shlomizadok/hammer-cli/pull/2

ares · 2016-11-16T15:21:13Z

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 · 2016-11-16T17:58:20Z

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

ares · 2016-11-16T20:15:25Z

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

mmoll · 2016-11-22T22:08:09Z

what's the status here?

tstrachota · 2016-11-24T12:40:30Z

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 · 2016-11-24T12:41:31Z

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

tstrachota · 2016-11-24T12:43:01Z

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

tstrachota · 2016-11-24T13:07:49Z

[test]

tstrachota · 2016-11-24T14:07:39Z

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

shlomizadok · 2016-11-28T13:45:04Z

@tstrachota rebased :)

mmoll · 2016-11-28T13:49:34Z

💚

tstrachota · 2016-11-28T13:50:37Z

Merged, thanks @shlomizadok and @mmoll !

theforeman-bot added Needs testing Not yet reviewed labels Sep 27, 2016

shlomizadok commented Sep 27, 2016

View reviewed changes

shlomizadok mentioned this pull request Sep 29, 2016

Added support for colorized values arches/table_print#45

Open

tstrachota reviewed Oct 4, 2016

View reviewed changes

tstrachota added Waiting on contributor and removed Needs testing Not yet reviewed labels Nov 14, 2016

theforeman-bot added Needs re-review Needs testing and removed Waiting on contributor labels Nov 15, 2016

mmoll mentioned this pull request Nov 17, 2016

package unicode-display_width gem (DEB) theforeman/foreman-packaging#1413

Merged

1 task

tstrachota removed the Needs re-review label Nov 24, 2016

Fixes #3520 - respect special chars width

ace478c

tstrachota merged commit 3a24459 into theforeman:master Nov 28, 2016

Conversation

shlomizadok commented Sep 27, 2016

Uh oh!

shlomizadok Sep 27, 2016

Choose a reason for hiding this comment

Uh oh!

ares Sep 27, 2016

Choose a reason for hiding this comment

Uh oh!

tstrachota Oct 4, 2016

Choose a reason for hiding this comment

Uh oh!

ares commented Sep 27, 2016

Uh oh!

shlomizadok commented Sep 28, 2016

Uh oh!

tstrachota left a comment

Choose a reason for hiding this comment

Uh oh!

tstrachota Oct 4, 2016

Choose a reason for hiding this comment

Uh oh!

tstrachota commented Nov 14, 2016

Uh oh!

domcleal commented Nov 14, 2016

Uh oh!

tstrachota commented Nov 14, 2016

Uh oh!

tstrachota commented Nov 14, 2016

Uh oh!

tstrachota commented Nov 15, 2016

Uh oh!

ares commented Nov 15, 2016

Uh oh!

tstrachota commented Nov 16, 2016

Uh oh!

tstrachota commented Nov 16, 2016

Uh oh!

ares commented Nov 16, 2016

Uh oh!

mmoll commented Nov 16, 2016

Uh oh!

ares commented Nov 16, 2016

Uh oh!

mmoll commented Nov 22, 2016

Uh oh!

tstrachota commented Nov 24, 2016

Uh oh!

mmoll commented Nov 24, 2016

Uh oh!

tstrachota commented Nov 24, 2016

Uh oh!

tstrachota commented Nov 24, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tstrachota commented Nov 24, 2016

Uh oh!

shlomizadok commented Nov 28, 2016

Uh oh!

mmoll commented Nov 28, 2016

Uh oh!

tstrachota commented Nov 28, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

tstrachota commented Nov 24, 2016 •

edited

Loading