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

Allows setting maximum column width --columns-width #307

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

Conversation

jm66
Copy link

@jm66 jm66 commented Oct 16, 2019

show_envvar=True,
help=(
'Columns custom width. '
'If specified truncates column values (default: auto)'
Copy link
Contributor

Choose a reason for hiding this comment

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

default is None here, right ? ..and how do I actually specify that if I wanted to ?

Copy link
Author

Choose a reason for hiding this comment

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

Was thinking something like this:

@click.option(
    '--columns-width',
    default=0,
    type=click.INT,
    envvar='HASS_COL_WIDTH',
    show_default=True,
    help=('Truncates column values (0: auto, -1: disable).'),
)

But I'm not pretty sure. Thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

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

I can't think of a better approach at the moment - implement it and update the PR and lets see.

@@ -99,7 +100,20 @@ def raw_format_output(
val = [match.value for match in fmtpair[1].find(item)]
row.append(", ".join(map(str, val)))
result.append(row)

# Truncates data
Copy link
Contributor

Choose a reason for hiding this comment

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

its crude and it somewhat works but I feel we should be able to do better than requiring users to pass in a width setting to get this.

How about using something like shutil.get_terminal_size() to get width and calculate a default max_width based on columns/terminal_width with an option to ignore it ?

Copy link
Contributor

Choose a reason for hiding this comment

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

then you could truly have a auto for column_width but should also have a -1 or max or something to have it just print everything as it does now.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for the feedback. What about this:

                terminal_size = shutil.get_terminal_size()
                number_c = min([len(r) for r in result])
                columns_width = int(terminal_size.columns / number_c)

Copy link
Contributor

Choose a reason for hiding this comment

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

looks about right - update the PR and we'll see.

@maxandersen
Copy link
Contributor

I like the idea - left some comments on how I think we can make it better and have a better auto default.

@@ -19,3 +19,4 @@
('CHANGED', 'last_changed'),
]
COLUMNS_SERVICES = [('DOMAIN', 'domain'), ("SERVICE", "domain.services[*]")]
COLUMNS_WIDTH_STR = "..."
Copy link
Author

Choose a reason for hiding this comment

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

Was considering to replace ... with the horizontal ellipsis \u2026. Thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

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

should be okey I think.

@jm66
Copy link
Author

jm66 commented Oct 23, 2019

PR updated. Thank you.

- If specified --columns-width or HASS_COL_WIDTH env var
  truncates column values
- Uses default const.COLUMNS_WIDTH_STR
- Fixes home-assistant-ecosystem#253
any other integer will be taken as fixed column
width. Default value is -1: disable since
truncating columns without user consent might be
a little too much.
0: auto does a basic calculation based on the
terminal size shutil.get_terminal_size() and the
number of columns in the result.
@maxandersen
Copy link
Contributor

I've rebased it to get the ci tests to hopefully pass!

@codecov-io
Copy link

codecov-io commented Nov 17, 2019

Codecov Report

Merging #307 into dev will decrease coverage by 0.03%.
The diff coverage is 71.42%.

Impacted file tree graph

@@            Coverage Diff             @@
##              dev     #307      +/-   ##
==========================================
- Coverage   75.25%   75.22%   -0.04%     
==========================================
  Files          22       22              
  Lines        1560     1574      +14     
==========================================
+ Hits         1174     1184      +10     
- Misses        386      390       +4
Impacted Files Coverage Δ
homeassistant_cli/const.py 100% <100%> (ø) ⬆️
homeassistant_cli/cli.py 76.66% <100%> (+0.53%) ⬆️
homeassistant_cli/helper.py 72.64% <60%> (-1.32%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1528c0a...b6b5dd2. Read the comment docs.

@stale
Copy link

stale bot commented Jan 17, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the wontfix label Jan 17, 2020
@stale stale bot removed the wontfix label Jan 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

allow setting maximum column width (feature req)
4 participants