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

Allow table rotation and resizing to be disabled #36

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

WilliamMcCumstie
Copy link

I've been having a couple of issues with the resize feature and external pagers. When rendering an extra large table with less, one of the two happens:

  1. When resize: false the table is rotated, or
  2. When resize: true the table is shrunk.

This occurs because the terminal width is not "technically increased" by the pager. However in practice the width is infinitely large. Instead it would be useful if the resizing and rotation feature can be disabled entirely.

I have updated ColumnConstraint to support a resize: nil option which disables the terminal width check. This causes the table to be rendered according to its nature_width. Then a pager can handle the extra wide tables.

Both the resize: true and resize: false options are unaffected by this change add the default is still false.

This allows tables to be integrated with an external pager
it 'ignores the maximum width constraint' do
expect(columns).not_to receive(:shrink)
column_widths = columns.enforce
expect(column_widths).to eql([2,2,2,2])

Choose a reason for hiding this comment

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

Layout/SpaceAfterComma: Space missing after comma.

context 'without resize (nil)' do
let(:options) { { width: 8, resize: nil }}

it 'ignores the maximum width constraint' do

Choose a reason for hiding this comment

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

Style/StringLiterals: Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.

@@ -54,6 +54,17 @@
expect(table.orientation.name).to eql(:vertical)
end
end

context 'without resize (nil)' do
let(:options) { { width: 8, resize: nil }}

Choose a reason for hiding this comment

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

Layout/SpaceInsideBlockBraces: Space missing inside }.

@@ -54,6 +54,17 @@
expect(table.orientation.name).to eql(:vertical)
end
end

context 'without resize (nil)' do

Choose a reason for hiding this comment

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

Style/StringLiterals: Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.

@@ -42,7 +42,7 @@
end
end

context 'without resize' do
context 'without resize (false)' do

Choose a reason for hiding this comment

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

Style/StringLiterals: Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.

@coveralls
Copy link

coveralls commented Nov 13, 2020

Coverage Status

Coverage increased (+0.007%) to 98.364% when pulling 9b326fc on openflighthpc:master into 052d272 on piotrmurach:master.

@piotrmurach
Copy link
Owner

Hi William,

Thanks for using tty-table and contributing.

Agreed, there should be a way to disable automatic rotation. I'm not sure though this is the approach I'd take to tackle this. The resize is a flag so I'm reluctant to allow for nil to be a valid value, plus nil carries no information as a value. How about adding a new rotate flag that by default would be set to true for backwards compatibility?

@WilliamMcCumstie
Copy link
Author

WilliamMcCumstie commented Nov 16, 2020

Okay I will look into making the change from resize: nil to some form of rotation flag.

Just one reservation about the naming of the new rotate flag. A rotate: true option sounds like it should explicitly trigger table rotation regardless of size. I am not in need of this feature however I can see scope for it in the future. Very least I don't wish to imply that is what it does.

How about a auto_rotate: true flag instead? That way the name more closely matches its purpose.

@piotrmurach
Copy link
Owner

I share your naming concern. Having said that, I usually stay away from naming options auto_ even if the behaviour is indeed automatic. If rotate was a method then I would completely agree that the intention is to rotate the table right then. However, with configuration options similar to resize option, the flag name indicates a general behaviour with intention explained in the docs. In the future, the behaviour may well change without the need for changing the flag name.

context 'without resize nor rotate' do
let(:options) { { width: 8, resize: false, rotate: false } }

it 'does not alter the column width nor table orientation' do

Choose a reason for hiding this comment

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

Style/StringLiterals: Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.

@@ -54,6 +54,17 @@
expect(table.orientation.name).to eql(:vertical)
end
end

context 'without resize nor rotate' do

Choose a reason for hiding this comment

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

Style/StringLiterals: Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.

@@ -42,8 +42,8 @@
end
end

context 'without resize' do
let(:options) { { width: 8, resize: false }}
context 'with rotate and without resize' do

Choose a reason for hiding this comment

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

Style/StringLiterals: Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.

@WilliamMcCumstie
Copy link
Author

Okay I've added the rotate flag as requested

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants