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

Better plumbing. #622

Merged
merged 4 commits into from
Apr 6, 2020
Merged

Better plumbing. #622

merged 4 commits into from
Apr 6, 2020

Conversation

bobthecow
Copy link
Owner

@bobthecow bobthecow commented Apr 4, 2020

This removes an implicit dependency on side effects from command line options, which was caused by name collisions with the base Console Application.

Things like --verbose and --no-interaction worked ... kind of. But not always (see #621, for example). And when it did work, it's only because both PsySH and the base Applcation were parsing argv and reading the options passed.

Improved plumbing, specifically:

  • When output decoration isn't explicitly specified, check whether stdout is piped.
  • When the interactive mode isn't explicitly specified, detect whether stdin is interactive.

Improved output:

  • Suppress startup messages when output is piped.
  • Write exceptions to stderr (to make it easier to split them off from desired output).
  • Suppress the little newline marker when --raw or piping stdout.

New configuration:

  • Add interactive mode configuration, update psy\info() output.
  • Add verbosity configuration, pass it through to ShellOutput.

Improved configuration:

  • Clean up color mode config code.
  • Move responsibility for command line input overrides into Configuration::fromInput
  • Move responsibility for input option declarations into Configuration::getInputOptions
  • Clean up command-line overrides for configuration.
  • Make config errors more consistent.

Drive by:

  • Add --ansi and --no-ansi aliases for color cli options (to match Symfony console and Composer).

@@ -384,6 +384,8 @@ public function testWriteStdout()

public function testWriteStdoutWithoutNewline()
{
$this->markTestSkipped('This test won\'t work on CI without overriding pipe detection');
Copy link
Contributor

Choose a reason for hiding this comment

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

This test is always skipped, even not on the CI?

Copy link
Owner Author

Choose a reason for hiding this comment

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

It'll fail locally if you pipe your phpunit output, too :)

I didn't really want a mock or an override or anything for the piped output detection, but I think I'm going to need it.

Copy link
Owner Author

Choose a reason for hiding this comment

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

It feels like there might be one more configuration option missing. Some way to say "force this to act like output isn't piped", just like we have "force this to decorate output" and "force this to act like input is interactive".

I haven't decided yet whether that's exactly the same as "force this to decorate output", but it might be.

Repository owner deleted a comment from codecov bot Apr 4, 2020
@codecov
Copy link

codecov bot commented Apr 4, 2020

Codecov Report

Merging #622 into master will increase coverage by 0.22%.
The diff coverage is 89.04%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #622      +/-   ##
============================================
+ Coverage     67.06%   67.28%   +0.22%     
============================================
  Files           129      127       -2     
  Lines          5207     5126      -81     
============================================
- Hits           3492     3449      -43     
+ Misses         1715     1677      -38
Impacted Files Coverage Δ Complexity Δ
src/functions.php 2.83% <0%> (+0.42%) 0 <0> (ø) ⬇️
src/Shell.php 75.39% <100%> (+1.44%) 0 <0> (-187) ⬇️
src/Configuration.php 80.34% <98.41%> (+9.77%) 0 <0> (-177) ⬇️
src/Sudo.php 0% <0%> (-100%) 0% <0%> (-7%)
src/CodeCleaner/ListPass.php 87.09% <0%> (-12.91%) 0% <0%> (-23%)
src/Command/TimeitCommand/TimeitVisitor.php 93.54% <0%> (-6.46%) 0% <0%> (-16%)
.../TabCompletion/Matcher/ObjectAttributesMatcher.php 76% <0%> (-4.77%) 0% <0%> (-8%)
src/TabCompletion/Matcher/ObjectMethodsMatcher.php 80.76% <0%> (-4.42%) 0% <0%> (-9%)
src/Command/EditCommand.php 36.5% <0%> (-3.8%) 0% <0%> (-19%)
...c/TabCompletion/Matcher/ClassAttributesMatcher.php 93.54% <0%> (-3.52%) 0% <0%> (-7%)
... and 61 more

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 9ff1074...72d5db9. Read the comment docs.

src/Shell.php Outdated
$this->writeVersionInfo();
$this->writeStartupMessage();
// Show a startup message, as long as output isn't piped.
if (!$this->config->outputIsPiped()) {
Copy link
Owner Author

Choose a reason for hiding this comment

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

After playing with it a bit, this isn't right. A forced interactive run should have a header, just like an implicitly interactive run. Pushing a fix for this.

@bobthecow
Copy link
Owner Author

This all feels pretty good, with the exception of the cli option names. I feel like --interaction (by analogy to --no-interaction) is just as awkward as --ansi rather than --color. Here's what feels best so far:

Psy Shell v0.10.2 (PHP 7.2.28 — cli)

Usage:
  psysh [--version] [--help] [files...]

Options:
  -h, --help            Display this help message.
  -c, --config FILE     Use an alternate PsySH config file location.
      --cwd PATH        Use an alternate working directory.
  -V, --version         Display the PsySH version.
      --color           Force colors in output.
      --no-color        Disable colors in output.
  -i, --interactive     Force PsySH to run in interactive mode.
  -n, --no-interactive  Run PsySH without interactive input. Requires input from stdin.
  -r, --raw-output      Print var_export-style return values (for non-interactive input)
  -q, --quiet           Shhhhhh.
  -v|vv|vvv, --verbose  Increase the verbosity of messages.

In addition to those, there are a bunch of aliases; --[no-]ansi for --[no-]color, --no-interaction for --no-interactive, to match all the Composer and Symfony console defaults, and -a for -i just so psysh -a does what you'd expect :P

@bobthecow bobthecow force-pushed the feature/better-plumbing branch from bd1df40 to f1320f0 Compare April 5, 2020 21:30
@bobthecow bobthecow force-pushed the feature/better-plumbing branch from f1320f0 to eb16fff Compare April 5, 2020 21:43
$config = new self(['configFile' => $input->getOption('config')]);

// Handle --color and --no-color (and --ansi and --no-ansi aliases)
if ($input->getOption('color') || $input->getOption('ansi')) {
Copy link
Owner Author

Choose a reason for hiding this comment

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

I'm feeling really good about this pull request. I think this is my last open question:

Should we use InputInterface::getOption?

Using getOption like we are here means things will blow up if we're handed input that hasn't been bound to a compatible input options definition. It is more correct, though. We just can't really enforce that people give us good input.

The more pragmatic approach is probably to use the underlying hasParameterOption / getParameterOption and do some more of the parsing and interpreting work manually, ourselves. It's not as clean, and not as guaranteed-correct, but it also doesn't rely on callers having already bound compatible options.

Either way, we'd still continue binding the input properly in bin/psysh when we call it. We'd just be more forgiving of other callers not doing the same.

I'm leaning towards the second option.

@GrahamCampbell what do you think? This is somewhat relevant to laravel/tinker#98 🙂

Copy link
Owner Author

Choose a reason for hiding this comment

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

K. I wen't with a third option.

I've got something that works either way, though definitely better with bound input. See 3c9b112.

It's a bit of a mess, but probably acceptably tucked away from end users that it won't be an issue?

@bobthecow bobthecow force-pushed the feature/better-plumbing branch 2 times, most recently from 4a5066b to fc6cb55 Compare April 6, 2020 05:20
 - Move responsibility for command line option declarations into
   `Configuration::getInputOptions`.
 - Move responsibility for overriding configuration via command line
   options into `Configuration::fromInput`.
 - Remove an implicit dependency on side effects from command line
   option name collisions with the base Console Application (this made
   `--verbose`, `--no-interaction`, and `--ansi` ... kind of work).

Update available command-line options:

 - `--interactive` and `--no-interactive` for interactive mode input.
 - `--color` and `--no-color` for decorated output.
 - `--verbose`, `--quiet` and `-v`/`-vv`/`-vvv` for output verbosity.

Add option aliases for compatibility with Symfony, Composer, and other
familiar tools:

 - `--ansi` and `--no-ansi` for `--color` and `--no-color`.
 - `--no-interaction` for `--no-interactive`.
 - `-i` and `-a` to force interactive mode input (hey there `php -a`).
@bobthecow bobthecow force-pushed the feature/better-plumbing branch from fc6cb55 to 72d5db9 Compare April 6, 2020 05:22
@bobthecow bobthecow merged commit 72d5db9 into master Apr 6, 2020
@bobthecow bobthecow deleted the feature/better-plumbing branch April 6, 2020 05:29
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