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

Adding support for a custom output format via run.echo #791

Merged
merged 1 commit into from May 30, 2021

Conversation

davidjmemmett
Copy link
Contributor

@davidjmemmett davidjmemmett commented Apr 26, 2021

The format of the output of fabric2 doesn't quite do it for me; rather than forcing a change that makes it different for everyone, I thought allowing users to customise the output format might be a good idea. It currently only supports wrapping the command passed to it, but I will happily look into ways of determining things like whether it's sudo or not (and maybe caching the current unadulterated command somewhere to display prettily like fabric1).

  • Added supporting test(s)
  • Added supporting documentation

Please let me know if there's anything else I've missed.

Cheers,
David

@bitprophet
Copy link
Member

bitprophet commented May 30, 2021

Thanks for this! Made a few changes, yell if you disagree, not sure when I will release.

  • Changed to echo_format to be a bit clearer, output_format sounded like it refers to stdout/err
  • It's not clear why you changed from print() to sys.stdout.write plus a newline in the new option.
    • Unless there's a strong upside to that which I missed, I'd rather keep it how it was, you never know who might've been mocking or overriding print() 🙃
    • but mostly, requiring a newline in the option is (IMHO) going to trip people up way more often than it would help somebody with the unusual case of "not wanting a newline at all" (is that a thing?)
      • yes, we could half and half this, and do "sys.stdout.write, plus an implicit newline" but again I don't get why that's better than print, especially in Python 3
  • Added changelog entry
  • Blackened (CI would've caught this if it was fixed yet. next thing on my agenda after diggin out my inbox...)

@@ -104,6 +104,7 @@ def basic_settings(self):
"hide": None,
"in_stream": None,
"out_stream": None,
"output_format": "\033[1;37m{command}\033[0m\n",
Copy link

@jcw jcw Jul 17, 2021

Choose a reason for hiding this comment

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

Triggered by the change in 1.6.0, I'd like to point out that this default is not very convenient for terminals with a white background. I constantly need to select the output before I can see what it is ... (MacOS, iTerm2) - please consider choosing anything but white as default.

Copy link
Member

Choose a reason for hiding this comment

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

To be fair this didn't truly change, it's been that way since inception.

  • It's hard to come up with defaults for this kind of thing that work equally well in the two broad color scheme options out there :( eg any sufficiently off-white gray that would be visible on a white BG becomes distressingly low-contrast on a black background. Other colors seem like a slightly garish default and again will often look horrible on one BG or the other depending on hue.
  • In my personal travels I see far more developers with black backgrounds than white so it's always seemed the slightly safer option. And users can now change it as needed thanks to the feature-add.

I suspect in the future if we grow additional colorized bits we'd extend it to have a "color scheme" config group so at least then one could package useful defaults for both and you'd only need to tweak one setting saying "light bg". FWIW.

Copy link

Choose a reason for hiding this comment

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

True (sorry, should have been more specific: the change caused me to look into the underlying code, been there all along). Light blue or light green, perhaps? Even the slightest hue would help. This problem is not specific to pyinvoke, clearly. I just wanted to raise a little flag, no more. Feel free to ignore.

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.

None yet

3 participants