ManageIQ / more_core_extensions

MoreCoreExtensions are a set of core extensions beyond those provided by ActiveSupport.
MIT License
5 stars 23 forks source link

remove color from length count #87

Closed d-m-u closed 4 years ago

d-m-u commented 4 years ago

It'd be cool if the length calculation here didn't include color codes.

also it's something that if it were right I expect Keenan might back

Fryguy commented 4 years ago

Can you show a before/after? I can't really understand what the issue was (i.e was the column too long or too short).

d-m-u commented 4 years ago

hey @Fryguy and @kbrock, thanks for the review on this, could I please ask you to take another look?

bdunne commented 4 years ago

Do we want to remove the color completely, or just for the field length?

d-m-u commented 4 years ago

it was only line 121, until the comment yesterday

and when I asked about it again, here's what I got:

Jul 07 17:31
that method totally looks right though
kbrock commented 4 years ago

thanks. this looks nice @d-m-u

work for you @Fryguy ?

Fryguy commented 4 years ago

@d-m-u Accidental close?

d-m-u commented 4 years ago

@Fryguy no, we're going to implement it in benchmark-sweet and not rely on this dependency.

Fryguy commented 4 years ago

ok...I still like the feature, so I'll probably reopen at some point.

Fryguy commented 4 years ago

Updated with the code snippet from https://github.com/ManageIQ/more_core_extensions/pull/87#discussion_r455286172 and added a bunch more tests for the edge cases.

@bdunne Please review.

bdunne commented 4 years ago

@Fryguy Can you add a test with multiple colors?

Fryguy commented 4 years ago

Updated with a better algorithm that determines matching escape sequences that fall into the expected width. Still need to add the specs.

miq-bot commented 4 years ago

Checked commits https://github.com/d-m-u/more_core_extensions/compare/e1ff5e04700bd530af177be648d617ff3a99166f~...4df5c3e26e7d03f2758cb10b592db2be97d79fbc with ruby 2.5.7, rubocop 0.69.0, haml-lint 0.28.0, and yamllint 2 files checked, 24 offenses detected

spec/core_ext/array/tableize_spec.rb