Make terminal table more responsive #7391

hjanuschka · 2016-12-08T21:55:38Z

this trys to fix the ugliness of terminal table on small terminals - by making tables fit to terminal width.
in addition to the variable size, i also did a middle_truncate.

so instead of very long va... now very ... value is displayed.

Large

Medium

Small

Ruby 1.9.3 added 'io/console' to the standard library.
the additional require should not be a problem.

to quickly test it use this ruby script:

require "fastlane_core"
FastlaneCore::PrintTable.print_values(config:
  { wwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwww: "bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb bbbbbbbbbbbbbbbbbbbbbbb bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb",
      wwwwwwwwwwwwwwwwwwwwwwwwxw: "ccc bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb bbbbbbbbbbbbbbbbbbbbbbb bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb" },

                                   hide_keys: [],
                                       title: "Summary SUM")

KrauseFx · 2016-12-09T00:45:42Z

fastlane_core/lib/fastlane_core/string_filters.rb

@@ -31,4 +31,14 @@ def truncate(truncate_at, options = {})

    "#{self[0, stop]}#{omission}"
  end
+
+  # Base taken from: http://stackoverflow.com/a/12202205/1945875
+  def middle_truncate(length = 20, options = {})


Is there a good way to detect the initial terminal width?

@KrauseFx
this is just a generic function.
in the table printer itself - the terminal width is used

tcols = IO.console.winsize[1]
max_length = tcols

Ah yeah, cool 👍

milch · 2016-12-12T20:13:32Z

This is a really good idea, however, I would also like an option to disable this (e.g. when running on CI) so that users have at least an option to see the whole thing for debugging purposes.

hjanuschka · 2016-12-12T22:48:09Z

@milch ok done -

truncate = false if Helper.is_ci? || FastlaneCore::Env.truthy?("DO_NOT_TRUNCATE_TABLES")

sorry for the force push, as i had to rebase for the truthy? stuff.

milch · 2016-12-13T00:32:29Z

sorry for the force push

No worries, I haven't reviewed yet 👍

FastlaneCore::Env.truthy?("DO_NOT_TRUNCATE_TABLES")

This is a bit nitpicky but mostly we'd like to prefix ENV-vars. Can we call it FL_NO_TRUNCATE_TABLES (or something)?

KrauseFx · 2017-01-11T09:53:48Z

fastlane_core/lib/fastlane_core/print_table.rb

@@ -18,7 +18,10 @@ def print_values(config: nil, title: nil, hide_keys: [], mask_keys: [])
        rows = self.collect_rows(options: options, hide_keys: hide_keys.map(&:to_s), mask_keys: mask_keys.map(&:to_s), prefix: '')

        params = {}
-        params[:rows] = limit_row_size(rows)
+        truncate = false if Helper.is_ci? || FastlaneCore::Env.truthy?("DO_NOT_TRUNCATE_TABLES")


Instead of DO_NOT, we've used SKIP_... until now

KrauseFx · 2017-01-11T09:56:34Z

This looks super promising, ideally it would also be used for the action table: #7811

hjanuschka · 2017-01-11T12:04:52Z

@KrauseFx feedback addressed.
should i do the action table thing in this PR or in a seperate?

hjanuschka · 2017-01-11T20:00:04Z

@KrauseFx ok i updated it to also work with newlines instead of the .. stuff (defaulting to: ....)

here is a sample to test:

require "fastlane_core"



FastlaneCore::PrintTable.print_values(config:
  { wwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwww: "bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb bbbbbbbbbbbbbbbbbbbbbbb bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb",
      wwwwwwwwwwwwwwwwwwwwwwwwxw: "ccc bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb bbbbbbbbbbbbbbbbbbbbbbb bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb" },

                                   hide_keys: [],
                                       title: "Summary SUM")


FastlaneCore::PrintTable.print_values(config:
                                         { wwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwww: "bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb bbbbbbbbbbbbbbbbbbbbbbb bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb",
                                             wwwwwwwwwwwwwwwwwwwwwwwwxw: "ccc bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb bbbbbbbbbbbbbbbbbbbbbbb bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb" },

                                                                          hide_keys: [],
                                                                              title: "Summary SUM",
                                                                              transform: :newline)

and i also added support for this in the actions_list.rb:

fastlane actions

and in detailed action descr.:

fastlane action adb

jinjorge · 2017-01-12T19:32:32Z

is there a way to make the line wraps happen at word boundaries?

from the screenshot above there are several words that are broken up from one line to the next

hjanuschka · 2017-01-13T10:00:57Z

@jinjorge good point, but i think there are cases where really long words would not be wrapped and resulting in a ugly table, you have a idea on how to solve that?

jinjorge · 2017-01-13T17:56:17Z

@hjanuschka It's a numbers game - there are a lot more sub 5-6 letter words than really long words. Probably best to optimize for the majority and see how things fall out.

To you question on how to solve it - I don't know, though I will do some research.

hjanuschka · 2017-01-23T21:29:11Z

test's pass now, ready for new review! @jinjorge i am unsure on how to solve it without breaking the output.

jinjorge · 2017-01-23T22:03:55Z

a regular expression match (\b) for word boundary.

hjanuschka · 2017-01-24T07:06:01Z

@jinjorge got it working !!! respecting whole word, but if too long forcing to break it! ;)

  # Base taken from: https://www.ruby-forum.com/topic/57805
  def wordwrap(length  = 80)
     self.gsub!(/(\S{#{length}})(?=\S)/, '\1 ')
     self.scan(/.{1,#{length}}(?:\s+|$)/)
  end

jinjorge · 2017-01-24T13:58:42Z

👏 👏 Awesome!!!

milch

Looks good to me aside from a couple minor things. I really think we should merge this soon

milch · 2017-02-01T13:58:33Z

fastlane_core/lib/fastlane_core/print_table.rb

+        return_array = []
+        tcols = IO.console.winsize[1]
+
+        col_count = rows.map(&:length).first || 1


Why is 1 being used as default here?

i used to use 0 - but well we have always atleast one column, otherwise it crashes later on due to a division by zero 💃

milch · 2017-02-01T14:00:27Z

fastlane_core/lib/fastlane_core/print_table.rb

+            value = col.to_s.dup
+            if transform == :truncate_middle
+              value = value.middle_truncate(max_value_length)
+            end


Use elsif here to more clearly show intent

good point, going to adress this once back in private life 👍

ahhh sorry to fast reply, i think there are cases where transform is false (e.g: CI run)
so its neither :newline nor :truncate_middle - any other ideas?

hjanuschka · 2017-02-01T18:14:22Z

@milch feedback addressed, as we have the super awesome full-word split now i enabled :newline by default, and therefore the elsif worked out pretty good 👍

endocrimes

I like where this is going 👍

endocrimes · 2017-02-10T20:40:33Z

fastlane_core/lib/fastlane_core/print_table.rb

@@ -24,7 +24,14 @@ def print_values(config: nil, title: nil, hide_keys: [], mask_keys: [])
        rows = self.collect_rows(options: options, hide_keys: hide_keys.map(&:to_s), mask_keys: mask_keys.map(&:to_s), prefix: '')

        params = {}
-        params[:rows] = limit_row_size(rows)
+        transform = false if FastlaneCore::Env.truthy?("SKIP_TRANSFORM_TABLES")


FL_SKIP_TABLE_TRANSFORM?

endocrimes · 2017-02-10T20:41:38Z

fastlane_core/lib/fastlane_core/print_table.rb


-        max_key_length = rows.map { |e| e[0].length }.max || 0
-        max_allowed_value_length = max_length - max_key_length - 7
        rows.map do |e|


rows.map do |row|? - single letter variable made it pretty difficult to infer when scan reading

true point, fixed

endocrimes · 2017-02-10T20:41:59Z

fastlane_core/lib/fastlane_core/print_table.rb

+        terminal_table_padding = 4
+        max_length = tcols - (col_count * terminal_table_padding)
+
+        max_value_length = (max_length / col_count)


you don't need parens here

can we keep them?, i like it a bit more from point of readability.

I tend to only prefer them when there are multiple expressions to make precedence easier to read, but sure.

endocrimes · 2017-02-10T20:49:21Z

fastlane_core/lib/fastlane_core/print_table.rb

+          new_row = []
+          e.each do |col|
+            value = col.to_s.dup
+            if transform == :truncate_middle


would be nice to break this out into a transform_row/column function, because then appending to the row will be in a single place and be easier to maintain and test

i.e

def transform_row(transform, row) row.map do |column| transform_column(transform, column) end end

def transform_column(transform, column) if transform == :truncate_middle ... value elseif transform == :newline ... value end

i am not 100% comfortable with this, as there is bunch a logic going on in :newline, colorization for example - lets keep it as it is, and try to finish this PR - as its a really long running one, and do the modularization in a seperate PR. (i just broke way too much with last minute quick-changes :sad:, last PRs)

hjanuschka

feedback adressed, master rebased and tty-screen used 👍

endocrimes · 2017-02-27T12:08:50Z

fastlane_core/lib/fastlane_core/print_table.rb

@@ -24,7 +24,14 @@ def print_values(config: nil, title: nil, hide_keys: [], mask_keys: [])
        rows = self.collect_rows(options: options, hide_keys: hide_keys.map(&:to_s), mask_keys: mask_keys.map(&:to_s), prefix: '')

        params = {}
-        params[:rows] = limit_row_size(rows)
+        transform = false if FastlaneCore::Env.truthy?("FL_SKIP_TABLE_TRANSFORM")


rather than overloading this, I'd maybe extract out a transform? method that is !FastlaneCore::Env.truthy?... and have the if statement be if transform? ... - this means you don't need to trace through mutating state.

endocrimes · 2017-02-27T12:11:12Z

fastlane_core/lib/fastlane_core/print_table.rb

+
+        rows.map do |row|
+          new_row = []
+          row.each do |col|


as we only return one value from this, we could do new_row = row.map do |col|

might also be easier to read if we break the contents of this block into a transform_column(column, truncation_style) method?

updated 👍

giginet · 2017-03-07T17:34:08Z

fastlane_core/lib/fastlane_core/print_table.rb

@@ -34,16 +41,60 @@ def print_values(config: nil, title: nil, hide_keys: [], mask_keys: [])
        return params
      end

-      def limit_row_size(rows, max_length = 100)
+      def colorize_array(array, colors)
+        value = ""


you can write this logic more clearly.

array.map { |l| "#{colors.first[0]}#{l}#{colors.last[0]}" unless colors.empty? }.join('\n')

I think this may not be equivalent, because it can create nil entries

mfurtak · 2017-03-27T13:40:33Z

Just checking in on the status of this PR - can the feedback be addressed? Everyone seems pretty positive about getting it in! 👍

KrauseFx · 2017-03-27T16:53:30Z

Improving all fastlane tables is definitely something we should fix, it's a big issue currently. @hjanuschka, is that something you want to finish up? 🚀

hjanuschka · 2017-03-28T07:42:33Z

@KrauseFx yeah i want to see this in.
however i worked alot on this, and the open feedback, is are just nit-picks, and i am afraid i break everything to resolve the feedback (which as a personal point of view, is not mission critical in this case)

so, how about merging this, and later on the feedback suggesters go on with it?

googlebot · 2017-03-28T07:42:36Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for the commit author(s). If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.

hjanuschka · 2017-03-28T07:44:23Z

@googlebot thx for this, i accidentally commited with corporate email - as soon as we know how to proceed with this PR i am going to rebase then it should be fine 👍

googlebot · 2017-04-03T17:22:10Z

CLAs look good, thanks!

hjanuschka · 2017-04-03T17:22:34Z

master rebased, cla stuff sorted out.

KrauseFx · 2017-04-07T02:03:35Z

fastlane_core/lib/fastlane_core/print_table.rb

@@ -34,16 +41,60 @@ def print_values(config: nil, title: nil, hide_keys: [], mask_keys: [])
        return params
      end

-      def limit_row_size(rows, max_length = 100)
+      def colorize_array(array, colors)


This method is never called, should it be part of this PR?

Whops, never mind, it is used

KrauseFx · 2017-04-07T02:06:17Z

fastlane_core/lib/fastlane_core/print_table.rb

+            new_row << value
+          end
+          return_array << new_row
+          return_array << :separator


Why do we have this here? This will result in a new line of dashes inbetween every row, making the table a lot bigger

KrauseFx · 2017-04-07T02:07:20Z

I think not having the separators for every row looks better 👍

KrauseFx · 2017-04-07T02:37:01Z

fastlane_core/spec/print_table_spec.rb

@@ -48,15 +48,15 @@

      value = FastlaneCore::PrintTable.print_values(config: @config, title: title, hide_keys: [:a_sensitive])
      expect(value[:title]).to eq(title.green)
-      expect(value[:rows]).to eq([['cert_name', "asdf"], ['output', '..'], ["a_bool", true]])
+      expect(value[:rows]).to eq([["cert_name", "asdf"], :separator, ["output", ".."], :separator, ["a_bool", "true"]])


It's clearly visible that the separator changes the existing behavior

Moved to #8809

KrauseFx · 2017-04-07T02:41:22Z

Moved to #8809 (by mistake), I addressed all of your feedback, thanks. Also some more tests and code style changes.

KrauseFx added the tool: fastlane_core label Dec 8, 2016

KrauseFx reviewed Dec 9, 2016

View reviewed changes

hjanuschka force-pushed the terminal_table_responsive branch from 512b1ad to 43de68a Compare December 11, 2016 18:02

hjanuschka force-pushed the terminal_table_responsive branch from 053150d to dfb45b1 Compare December 12, 2016 22:47

KrauseFx changed the title ~~make terminal table resposnive, and do middle_truncate~~ Make terminal table more responsive Dec 13, 2016

KrauseFx mentioned this pull request Jan 11, 2017

[actions] cli output formatting for fastlane action changelog_from_git_commits #7811

Closed

2 tasks

KrauseFx reviewed Jan 11, 2017

View reviewed changes

fastlane-bot added the status: needs-attention label Jan 14, 2017

hjanuschka force-pushed the terminal_table_responsive branch 2 times, most recently from b74e04e to e3fdf48 Compare January 23, 2017 21:28

milch previously requested changes Feb 1, 2017

View reviewed changes

hjanuschka mentioned this pull request Feb 6, 2017

[WIP]: change terminal-table to tty-table #8137

Closed

2 tasks

endocrimes previously requested changes Feb 10, 2017

View reviewed changes

hjanuschka force-pushed the terminal_table_responsive branch from 3314050 to c4ebdfb Compare February 27, 2017 09:33

hjanuschka commented Feb 27, 2017

View reviewed changes

endocrimes reviewed Feb 27, 2017

View reviewed changes

giginet previously requested changes Mar 7, 2017

View reviewed changes

googlebot added the cla: no label Mar 28, 2017

make terminal table resposnive

1b0b09b

hjanuschka force-pushed the terminal_table_responsive branch from c4ebdfb to 1b0b09b Compare April 3, 2017 17:22

googlebot added cla: yes and removed cla: no labels Apr 3, 2017

KrauseFx self-assigned this Apr 3, 2017

KrauseFx reviewed Apr 7, 2017

View reviewed changes

KrauseFx mentioned this pull request Apr 7, 2017

Make all tables in fastlane more responsive to the terminal width #8809

Closed

KrauseFx closed this Apr 7, 2017

fastlane-bot removed the status: needs-attention label Apr 7, 2017

hjanuschka mentioned this pull request Apr 8, 2017

Make all tables in fastlane more responsive to the terminal width #8830

Merged

fastlane locked and limited conversation to collaborators Jul 7, 2017

Make terminal table more responsive #7391

Make terminal table more responsive #7391

Conversation

hjanuschka commented Dec 8, 2016 • edited Loading

Large

Medium

Small

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

milch commented Dec 12, 2016

hjanuschka commented Dec 12, 2016

milch commented Dec 13, 2016

Choose a reason for hiding this comment

KrauseFx commented Jan 11, 2017

hjanuschka commented Jan 11, 2017

hjanuschka commented Jan 11, 2017

jinjorge commented Jan 12, 2017

hjanuschka commented Jan 13, 2017

jinjorge commented Jan 13, 2017

hjanuschka commented Jan 23, 2017

jinjorge commented Jan 23, 2017

hjanuschka commented Jan 24, 2017

jinjorge commented Jan 24, 2017 • edited Loading

milch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hjanuschka commented Feb 1, 2017

endocrimes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hjanuschka left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mfurtak commented Mar 27, 2017

KrauseFx commented Mar 27, 2017

hjanuschka commented Mar 28, 2017

googlebot commented Mar 28, 2017

hjanuschka commented Mar 28, 2017

googlebot commented Apr 3, 2017

hjanuschka commented Apr 3, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KrauseFx commented Apr 7, 2017

Choose a reason for hiding this comment

KrauseFx commented Apr 7, 2017

hjanuschka commented Dec 8, 2016 •

edited

Loading

jinjorge commented Jan 24, 2017 •

edited

Loading

hjanuschka left a comment •

edited

Loading