Use sprintf around formatted strings #7
Conversation
|
👍 |
| sprintf( | ||
| 'Parameter of type %s is invalid; must be MongoClient or Mongo', | ||
| (is_object($mongo) ? get_class($mongo) : gettype($mongo)) | ||
| ) |
There was a problem hiding this comment.
ZF2 uses a slightly different indentation:
throw new InvalidArgumentException(sprintf(
'Parameter of type %s is invalid; must be MongoClient or Mongo',
(is_object($mongo) ? get_class($mongo) : gettype($mongo))
));There was a problem hiding this comment.
Where is this documented?
There was a problem hiding this comment.
Where is this documented?
Good question for a framework where IMHO documentation is a problem 😄
Looking at other ZF2 code in almost every situation the indentation I mentioned is used: https://github.com/zendframework/zend-session/search?utf8=%E2%9C%93&q=Exception%28sprintf%28
There was a problem hiding this comment.
I think it may for consistency instead of standard
There was a problem hiding this comment.
Personally, I think the way you've suggested indenting it is what caused this error to occur. It's obvious to me that someone's written their code, and copy paste/modified the arguments within, and not noticed the sprintf tucked away :)
There was a problem hiding this comment.
The error is caused by the fact the original PR was merged without proper tests: zendframework/zendframework#3221 If there was a unittest for this exception the problem would have been detected immediately, that is why it is important to add unittests. BTW a unittest is missing for this PR too 😉
|
Nice catch :) Yes; we certainly need some tests around this guy. |
|
Added Tests, Coveralls doesn't get it because |
|
|
||
| $expectedExceptionMessage = sprintf( | ||
| 'Parameter of type %s is invalid; must be MongoClient or Mongo', | ||
| get_class($notMongo) |
There was a problem hiding this comment.
Expectation should be static and not be built with the same logic as the implementation
|
👍 For add the test |
| 'database' => 'zf2_tests', | ||
| 'collection' => 'sessions', | ||
| ] | ||
| ); |
There was a problem hiding this comment.
Previous indentation was the right one.
There was a problem hiding this comment.
Indeed however I'll preffer don't add more noise in the PR for just cosmetic changes.
There was a problem hiding this comment.
No problem. I guess it will be corrected when it is merged?
There was a problem hiding this comment.
no. The only wrong thing is to change unrelated lines in the PR. Both indentations are correct.
There was a problem hiding this comment.
Oops, sorry, I thought your comment was to me, but it was for @Mezzle
Use sprintf around formatted strings
No description provided.