Conversation
composer.json
Outdated
| "require": { | ||
| "php" : ">=7.1", | ||
| "beberlei/assert": "^2.4", | ||
| "php-school/terminal": "dev-master", |
There was a problem hiding this comment.
Needs updating when we tag first release.
| sprintf('Terminal "%s" is not a valid TTY', $this->terminal->getDetails()) | ||
| ); | ||
| if (!$this->terminal->isInteractive()) { | ||
| throw new InvalidTerminalException('Terminal is not interactive (TTY)'); |
There was a problem hiding this comment.
This is what I mentioned in the php-school/terminal PR. If it's not a valid TTY - then there is no name.
Codecov Report
@@ Coverage Diff @@
## master #81 +/- ##
============================================
+ Coverage 89.13% 96.49% +7.36%
- Complexity 246 288 +42
============================================
Files 18 22 +4
Lines 690 885 +195
============================================
+ Hits 615 854 +239
+ Misses 75 31 -44
Continue to review full report at Codecov.
|
|
|
||
| return array_map(function ($row) use ($setColour, $unsetColour) { | ||
| return sprintf( | ||
| "%s%s%s%s%s%s%s\n\r", |
There was a problem hiding this comment.
dropped this \r because we don't support windows anymore and phpstorm keep changing the line endings when I was editing the test files which was mega annoying.
| $this->emptyRow(); | ||
|
|
||
| $confirmText = sprintf(' < %s > ', $confirmText); | ||
| $leftFill = ($promptWidth / 2) - (mb_strlen($confirmText) / 2); |
There was a problem hiding this comment.
All these changes before the while loop are just refactorings, no logic change
|
|
||
| public function ask() : InputResult | ||
| { | ||
| $this->inputIO->registerControlCallback(InputCharacter::UP, function (string $input) { |
There was a problem hiding this comment.
If the user presses up while they are giving a number (and the current input is actually a number) then we shift it up 1
There was a problem hiding this comment.
we dont validate until they hit enter and therefore if they enter a string we can't really increment it
|
|
||
|
|
||
| [44;37m [49;39m | ||
| [44;37m PHP School FTW [49;39m |
There was a problem hiding this comment.
all these text file changes are just removing \r
src/Input/Number.php
Outdated
|
|
||
| public function validate(string $input) : bool | ||
| { | ||
| return (bool) preg_match('/^\d+$/', $input); |
There was a problem hiding this comment.
this fails negative numbers, not sure but I'd think we'd want to accept that right?
There was a problem hiding this comment.
Yeah defo - will fix up.
| { | ||
| if ($this->validator) { | ||
| $validator = $this->validator; | ||
| return $validator($input); |
There was a problem hiding this comment.
1 thing I noticed here was that a developer could potentially have quite a complex validator added to an input with 1 or more "rules"... however there's no way to feedback from that validator the error that occurred, in this case they'd need to create a new input type
What do you think, worth refactoring a little for that ?
There was a problem hiding this comment.
Yeah I thought about this but was being lazy. Probably allow to throw an exception and use the exception message, bit like how symfony does.
mikeymike
left a comment
mikeymike
left a comment
There was a problem hiding this comment.
Overall man I love this, I did however still manage to break it to produce this
I did that by doing alt+delete then inputting as normal
This one I did by checking if maybe alt+delete was somewhat equal to the standard ctrl+u which deletes lines etc as I know I have alt+delete in my terminal mapped to delete words
I believe ctrl+p and other random shortcuts lead to similar behaviour ... wonder if there's a list of standard bindings we'd need to cater for ?
|
@mikeymike damn you for breaking! - will take a look |
|
@mikeymike should all be addressed now - we just ignore all controls now, and allow handling specific ones. For ctrl +p, ctrl +u we just plain ignore. We can add support for them in the future if we wanted as I mentioned in php-school/terminal#5 |
|
❤️ merging |
3 participants
php-school/terminal- currently dev-master we will change that before merging or next release anywayThis PR is quite long, but there should be some stuff that can be ignored for review, will annotate.