Fix incorrect docblock var and return types #264
Conversation
| @@ -249,7 +249,7 @@ private function validateOptions(array $options) | |||
| if (false === is_object($provider) && false === is_string($provider)) { | |||
| throw new \InvalidArgumentException(sprintf( | |||
| 'The provider should be a string or an object, got %s instead', | |||
| is_scalar($provider) ?$provider :gettype($provider) | |||
| is_scalar($provider) ? $provider : gettype($provider) | |||
There was a problem hiding this comment.
Personally, I prefer it as well. We do have a CS checker, but I'm not sure if it handles this situation.
There was a problem hiding this comment.
It's the result of php-cs-fixer run with rules you configured in .php_cs
|
+1 for this PR, good work |
| @@ -22,7 +22,7 @@ class ListName implements MethodInterface | |||
| */ | |||
| public function canBuild($name) | |||
| { | |||
| return preg_match('#\{([^,]+(\s*,\s*[^,]+)*)\}#', $name, $this->matches); | |||
| return 1 === preg_match('#\{([^,]+(\s*,\s*[^,]+)*)\}#', $name, $this->matches); | |||
There was a problem hiding this comment.
I would rather see these kinds of things in a separate PR - it's not that they aren't obviously useful, but it's a lot of chatter that makes it much more difficult to review and merge quickly.
|
Overall, really great fixes - documentation and annotations are probably some of the biggest struggles we've had with it so far, thanks for all this. Re: my comments about separate PRs, for these really public repos, I vastly prefer breaking things down as granularly as possible. Since it's infrequent that I have large blocks of time to review things, it's much better to have a small set of homogeneous changes than many varied changes in one PR. I like everything that you are doing, but now when you go through and make changes based on the conversation above, I have to review all 22 files all over again, which takes a larger chunk of time than I might have for a while. |
|
OK, i've removed changes not related to docblocks. |
|
@tshelburne I've merge master and resolved conflict, so this PR can be merged now. |
|
I would rather see you rebase up to commit 607c3b9 off of master so that we don't end up with two merge commits. Make sense? |
|
@tshelburne done |
| @@ -25,7 +25,7 @@ class ListName implements MethodInterface | |||
| */ | |||
| public function canBuild($name) | |||
| { | |||
| return 1 === preg_match('#\{([^,]+(\s*,\s*[^,]+)*)\}#', $name, $this->matches); | |||
There was a problem hiding this comment.
Looks like you might have selected the wrong thing in rebase.
|
@Seldaek I'm happy with this, but it's a lot of "preference" - you good? |
|
@tshelburne this could be quickly be rebased and merged :) |
|
@theofidry @tshelburne Rebased |
Fix incorrect docblock var and return types
|
@munkie Thanks! |
This PR fixes: