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

[2.x] Forward input options to psysh #98

Merged
merged 2 commits into from
Apr 7, 2020
Merged

[2.x] Forward input options to psysh #98

merged 2 commits into from
Apr 7, 2020

Conversation

GrahamCampbell
Copy link
Member

@GrahamCampbell GrahamCampbell commented Apr 5, 2020

This PR forwards things like --no-ansi and -vv to psysh in the proper way, instead of ignoring them. Once this PR is merged, we can tag tinker v2.4.0.

// cc @bobthecow

@GrahamCampbell
Copy link
Member Author

GrahamCampbell commented Apr 5, 2020

Heh, our tests shouldn't have passed. I guess we are not actually testing the tinker command. ;)

@GrahamCampbell
Copy link
Member Author

Hmm, asking for verbose seems to just hang forever?

image

@bobthecow
Copy link
Contributor

It's not hanging forever. It's just running as --quiet.

$input->getOption('verbose') is returning true, which PHP matches to anything truthy in this switch because PHP is awesome:

https://github.com/bobthecow/psysh/blob/5f97c26be75a2334a08530e7f99ead9cf0137a21/src/Configuration.php#L268

Apparently your verbose isn't compatible with my verbose. This seems at odds with what it spits out for --help on the command line:

Screen Shot 2020-04-06 at 12 51 54 PM

Adding explicit handling for === true fixes it, and that seems like the reasonable answer, so I'll do that. But you should track down what's going on with your --verbose :)

bobthecow added a commit to bobthecow/psysh that referenced this pull request Apr 6, 2020
@bobthecow
Copy link
Contributor

Confirmed that the psysh change fixed running with -v.

@GrahamCampbell
Copy link
Member Author

Oh, lol. Our verbose is just whatever symfony does as the default for commands.

@GrahamCampbell
Copy link
Member Author

Is there a way for psysh to match up with those, or is it intentionally not defined like that?

@bobthecow
Copy link
Contributor

bobthecow commented Apr 6, 2020

PsySH does match up (with how Console\Application handles it, maybe something else does it differently?)

https://github.com/symfony/console/blob/619054da7627df1c2169c7aabe0df78f9320847c/Application.php#L869-L883

@bobthecow
Copy link
Contributor

Oh cool. Symfony used the wrong definition for how they actually implemented the handling:

https://github.com/symfony/console/blob/619054da7627df1c2169c7aabe0df78f9320847c/Application.php#L975

@bobthecow
Copy link
Contributor

Here we go. When the levels for --verbose flags were first added, it was defined as VALUE_OPTIONAL, as it needs to be for it to work with the values they're actually parsing:

symfony/console@039ba6f

But the same day, it was changed to VALUE_NONE with the commit message "Fix tests" 😬

symfony/console@841817b

… so the input definition has been broken the entire time, and the only reason it mostly works is Symfony doesn't actually use the input definition to parse the input for --verbose.

Given this, I think the answer is to stop trying to use getOption at all for verbosity, because using the bound input option will give the wrong answer.

@bobthecow
Copy link
Contributor

All better! bobthecow/psysh@4884edf

@GrahamCampbell
Copy link
Member Author

@bobthecow Thanks for this. The no-interaction flag doesn't seem to work, however:

image

@GrahamCampbell
Copy link
Member Author

It looks like it does get set to disabled, but the configuration later doesn't get respected?

@GrahamCampbell GrahamCampbell changed the title [WIP] [2.x] Forward input options to psysh [2.x] Forward input options to psysh Apr 7, 2020
@driesvints
Copy link
Member

Why would you use --no-interaction with Tinker?

@GrahamCampbell
Copy link
Member Author

Why would you use --no-interaction with Tinker?

Well, all laravel's commands should respect the no interaction flag. In the case of the tinker command, instead of reading from the input "live", we'd just one-time read whatever was piped into from stdin, and execute it.

@GrahamCampbell
Copy link
Member Author

I'd say this PR is ready for merge now. Handling of no-interaction is to be handled outside of the tinker package. We already forward the config properly.

@bobthecow
Copy link
Contributor

It (probably) is doing "no interaction", you're just putting it in a weird place by not giving it anything to execute.

Screen Shot 2020-04-07 at 7 01 25 AM

@taylorotwell taylorotwell merged commit aa7a467 into 2.x Apr 7, 2020
@GrahamCampbell GrahamCampbell deleted the input-options branch April 7, 2020 14:12
@GrahamCampbell
Copy link
Member Author

It (probably) is doing "no interaction", you're just putting it in a weird place by not giving it anything to execute.

Surely psysh should either exit straight away with code 0 in that case, or it should throw an exception saying nothing to do?

@bobthecow
Copy link
Contributor

It's a pretty straightforward change. I'm not entirely convinced it's what people will expect though :)

Screen Shot 2020-04-07 at 8 03 08 AM

@GrahamCampbell
Copy link
Member Author

Isn't the whole point of no-interaction is that the process will never wait for input?

bobthecow added a commit to bobthecow/psysh that referenced this pull request May 2, 2020
…vided

Previously we'd hang out trying to get input from readline, which is very much not what you'd expect if you explicitly asked for `--no-interaction` :)

See laravel/tinker#98
bobthecow added a commit to bobthecow/psysh that referenced this pull request May 2, 2020
…vided

Previously we'd hang out trying to get input from readline, which is very much not what you'd expect if you explicitly asked for `--no-interaction` :)

See laravel/tinker#98
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