Skip to content
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

Format tables including full width characters properly #919

Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions cucumber.gemspec
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ Gem::Specification.new do |s|
s.add_development_dependency 'coveralls', '~> 0.7'
s.add_development_dependency 'syntax', '>= 1.0.0'
s.add_development_dependency 'pry'
s.add_development_dependency 'unicode'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm wondering why this is a development dependency. Won't we need it at runtime too? Or is the idea that we'll expect users to have to have installed it themselves if they want to benefit from it?


# For Documentation:
s.add_development_dependency 'bcat', '~> 0.6.2'
Expand Down
1 change: 1 addition & 0 deletions lib/cucumber.rb
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
module Cucumber
class << self
attr_accessor :wants_to_quit
attr_accessor :treats_ambiguous_as_fullwidth
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think having this option is fine, but we need to add it to https://github.com/cucumber/cucumber-ruby/blob/master/lib/cucumber/configuration.rb instead.

This has implications for how we use it in the DataTable, but we can figure that out.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also suggest we rename it to treat_ambiguous_width_characters_as_double_width to be crystal clear.


def logger
return @log if @log
Expand Down
3 changes: 2 additions & 1 deletion lib/cucumber/formatter/pretty.rb
Original file line number Diff line number Diff line change
Expand Up @@ -203,7 +203,8 @@ def table_cell_value(value, status)
status ||= @status || :passed
width = @table.col_width(@col_index)
cell_text = escape_cell(value.to_s || '')
padded = cell_text + (' ' * (width - cell_text.unpack('U*').length))
text_width = defined?(Unicode) ? Unicode.width(cell_text, Cucumber.treats_ambiguous_as_fullwidth) : cell_text.unpack('U*').length
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code looks quite similar to what's in DataTable::Cells#width below. If we move that logic into a Cell#width method, could we re-use that logic here by constructing an instance of a Cell?

padded = cell_text + (' ' * (width - text_width))
prefix = cell_prefix(status)
@io.print(' ' + format_string("#{prefix}#{padded}", status) + ::Cucumber::Term::ANSIColor.reset(" |"))
@io.flush
Expand Down
13 changes: 12 additions & 1 deletion lib/cucumber/multiline_argument/data_table.rb
Original file line number Diff line number Diff line change
Expand Up @@ -692,7 +692,18 @@ def index
end

def width
map{|cell| cell.value ? escape_cell(cell.value.to_s).unpack('U*').length : 0}.max
map {|cell|
if cell.value
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's extract a Cell#width method here and call it.

escaped = escape_cell(cell.value.to_s)
if defined?(Unicode)
Unicode.width(escaped, Cucumber.treats_ambiguous_as_fullwidth)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of using the global option here, let's accept this specific option as a constructor parameter (using an options hash or keyword argument would be best IMO) into the DataTable and pass it down to the Cell.

Wherever we construct instances of DataTable we'll almost certainly have access to an instance of the configuration where we can read that setting from.

else
escaped.unpack('U*').length
end
else
0
end
}.max
end
end

Expand Down
85 changes: 85 additions & 0 deletions spec/cucumber/formatter/unicode_pretty_spec.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,85 @@
# coding: utf-8

require 'spec_helper'
require 'cucumber/formatter/spec_helper'
require 'cucumber/formatter/pretty'
require 'cucumber/cli/options'

unless Cucumber::JRUBY
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why this logic? Does the unicode gem not work for JRuby?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

require 'unicode'

module Cucumber
module Formatter
describe Pretty do
extend SpecHelperDsl
include SpecHelper

before(:each) do
Cucumber::Term::ANSIColor.coloring = false
@out = StringIO.new
@formatter = Pretty.new(runtime, @out, {})
end

describe "with a multiline step arg including fullwidth characters" do
define_feature <<-FEATURE
Feature: Traveling circus

Scenario: Monkey goes to town
Given there are monkeys:
| 名前 |
| キュウリ |
| Cucumber |
FEATURE

it "displays the multiline string" do
run_defined_feature

expect(@out.string).to include <<OUTPUT
Given there are monkeys:
| 名前 |
| キュウリ |
| Cucumber |
OUTPUT
end
end

describe "with a multiline step arg including ambiguous width characters" do
define_feature <<-FEATURE
Feature: Traveling circus

Scenario: Monkey goes to town
Given there are monkeys:
| ∀ |
| forall |
FEATURE

after :each do
Cucumber.treats_ambiguous_as_fullwidth = nil
end

it "prints as fullwidth" do
Cucumber.treats_ambiguous_as_fullwidth = true
run_defined_feature

expect(@out.string).to include <<OUTPUT
Given there are monkeys:
| ∀ |
| forall |
OUTPUT
end

it "prints as halfwidth" do
Cucumber.treats_ambiguous_as_fullwidth = false
run_defined_feature

expect(@out.string).to include <<OUTPUT
Given there are monkeys:
| ∀ |
| forall |
OUTPUT
end
end
end
end
end
end