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

Increase output width of all commands #643

Merged
merged 1 commit into from
May 23, 2022

Conversation

rudyardrichter
Copy link
Contributor

Use the context_settings argument to click.Command to adjust max content width to 80% of the terminal size.

@rudyardrichter rudyardrichter requested a review from sirosen May 23, 2022 19:06
@rudyardrichter rudyardrichter force-pushed the increase-content-width branch from ae4c862 to 6444b12 Compare May 23, 2022 19:07
term_size = get_terminal_size(fallback=(80, 20))
kwargs["context_settings"]["max_content_width"] = int(
0.8 * term_size.columns
)
Copy link
Member

Choose a reason for hiding this comment

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

One thing which surprised me with this change is that globus transfer -h | cat became narrower.
I'm wondering if we should "widen out" if the terminal width is small, which will also cover the fallback case.

So, something like (and don't mind exactly how I write this please 😉 ):

cols = get_terminal_size(fallback=...).columns
if cols < 100: cols
else: cols * 0.8

Does that seem nice?

Copy link
Contributor Author

@rudyardrichter rudyardrichter May 23, 2022

Choose a reason for hiding this comment

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

Oh, right. The fallback will take over for the pipe case, so we don't want to again take 80% of that.

I also wonder about cases where get_terminal_size could fail; I figure it doesn't hurt to catch those.

Copy link
Member

Choose a reason for hiding this comment

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

Just looking at the py3.10 source for get_terminal_size I think this is already an "error handling variant" of os.get_terminal_size(sys.stdout.fileno()).

I would assume that it works as expected and we can handle any bug reports if someone finds a context where it does not. It doesn't look like there have been changes there since py3.5, so I bet it's pretty solid.

@sirosen
Copy link
Member

sirosen commented May 23, 2022

I dropped the helptext from globus transfer -h before and after into files for a side-by-side (copy-paste, to avoid shutil shenanigans):

$ wc -l narrow.txt
136 narrow.txt
$ wc -l wide.txt
82 wide.txt

I think that's a huge improvement, and it's equally striking with commands that have shorter helptext.

I have a change to the --jmespath helptext I plan to suggest as a follow-up, to make that option help fit on one line in more cases.

example helptext with shorter jmespath help

Compare this:

$ globus whoami -h
Usage: globus whoami [OPTIONS]

  Display information for the currently logged-in user.

Options:
  --linked-identities            Also show identities linked to the currently logged-in primary identity.
  -v, --verbose                  Control level of output
  -h, --help                     Show this message and exit.
  -F, --format [unix|json|text]  Output format for stdout. Defaults to text
  --jmespath, --jq TEXT          A JMESPath expression to apply to json output. Forces the format to be json processed by this expression

vs main:

$ globus whoami -h
Usage: globus whoami [OPTIONS]

  Display information for the currently logged-in user.

Options:
  --linked-identities            Also show identities linked to the currently
                                 logged-in primary identity.
  -v, --verbose                  Control level of output
  -h, --help                     Show this message and exit.
  -F, --format [unix|json|text]  Output format for stdout. Defaults to text
  --jmespath, --jq TEXT          A JMESPath expression to apply to json
                                 output. Forces the format to be json
                                 processed by this expression

I would like to have the text widen to full width on very narrow terminals -- since I think that's an easy improvement -- and then we can merge.

@rudyardrichter rudyardrichter force-pushed the increase-content-width branch from 9542a7a to efe8829 Compare May 23, 2022 19:46
Copy link
Member

@sirosen sirosen left a comment

Choose a reason for hiding this comment

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

I see the change I asked for is in. Although a somewhat low-key improvement, I'm quite excited for this!

@sirosen sirosen merged commit e76fe10 into globus:main May 23, 2022
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.

2 participants