Introduction of Grumphp #24
Conversation
- GrumPHP Composer validation: The version field is present, it is recommended to leave it out if the package is published on Packagist.
.php_cs
Outdated
| ], | ||
| 'linebreak_after_opening_tag' => true, | ||
| 'no_short_echo_tag' => true, | ||
| 'no_useless_else' => true, |
There was a problem hiding this comment.
The choice for using else everywhere was deliberate and is to improve readability. Please set this to false and re-add removed else statements.
There was a problem hiding this comment.
I did not expect this feedback/response, I would suggest you to see the video of @rdohms of his talk "Writing code that lasts" … or writing code you won’t hate tomorrow. Where Rafael clearly and visually explains what benefits to achieve.
- https://youtu.be/rMW6MZIREgg?t=32m5s (4 minutes)
There was a problem hiding this comment.
@cafferata @qurben I actually believe we should after all get rid of these extra else statements. They do look like noise to the most of the developer community, and the members of developer community are our most valuable contributors.
@andrederoos shall we switch to the modern code standard and get rid of all these redundant else's? With short methods involving little branching (like what we have) it makes especially good sense. (Also, the guy on the video has his point!)
There was a problem hiding this comment.
the guy in the video warns for nesting ifs in ifs etc. like a christmas tree :) I totally agree with that. Each function should have a single responsibility, therefor unlikely to find nested if/else in our code base (hopefully ;-).
So why do we use it then?
- as stated earlier, readability
- but MORE important, by closing the if/else you show in your code that you are aware of every situation that can occur and act accordingly.
There was a problem hiding this comment.
@qurben @andrederoos, @dnl-blkv if you prefer to keep the else statements, I will remove the commit. But in my opinion, a difference of 1700 lines of code is also readability. If 'this' situation occurs, then we will do 'this'. In all other cases, the default flow will be handled. So, when you make an opinion, I will apply it and close the further discussion.
There was a problem hiding this comment.
@cafferata 1,568 of those lines of code is from deletion of composer.lock... Which is by the way already deleted (weird)... :D
There was a problem hiding this comment.
@cafferata thanks! lets keep the if/else for now.
There was a problem hiding this comment.
I've deleted the two commits (no_useless_else / phpdoc_order).
grumphp.yml
Outdated
| exclude: ['vendor'] | ||
| jobs: ~ | ||
| triggered_by: ['php', 'phtml'] | ||
| phpstan: |
There was a problem hiding this comment.
Does GrumPhp trigger PhpStan when used with PHP 5.6?
We currently still support both 5.6 and 7.0, PhpStan is a dev dependency which doesn't need to be run.
There was a problem hiding this comment.
It doesn't matter if GrumPHP PhpStan triggered, it's because PhpStan has a dependency to PHP 7. When installing Composer (with dev dependency), you immediately get a notification that your PHP version does not satisfy the requirements. These is introduced with commit: 7e8fad5
- Installation request for phpstan/phpstan 0.8 -> satisfiable by phpstan/phpstan[0.8].
- phpstan/phpstan 0.8 requires php ~7.0 -> your PHP version (5.6.30) does not satisfy that requirement.
There was a problem hiding this comment.
Thanks for pointing this out. We removed Phpstan because of this. It was not intentional to stop supporting php 5.6
There was a problem hiding this comment.
Oké cool, did the same with this pull request. :-)
composer.json
Outdated
| @@ -40,20 +39,12 @@ | |||
| "require-dev": { | |||
| "phpunit/phpunit": "^5.7", | |||
| "friendsofphp/php-cs-fixer": "^2.4", | |||
| "phpstan/phpstan": "^0.8" | |||
| "phpstan/phpstan": "^0.8.0", | |||
There was a problem hiding this comment.
So this one indeed should go because we don't want to kill 5.6 compatibility.
There was a problem hiding this comment.
It's already gone! :-)
src/Context/ApiContext.php
Outdated
| * @throws BunqException | ||
| * @return string[][] |
There was a problem hiding this comment.
Our normal order is: first @return then @throws, motivated by the fact that the return is a normal and expected behaviour, whereas @throws is optional and exceptional. It makes sense to first highlight the main flow and then say "oh, and in case something goes wrong...".
@cafferata what's your point in changing this order?
There was a problem hiding this comment.
-
It's an purpose PSR-5 standard by the Framework Interop Group.
(https://github.com/php-fig/fig-standards/blob/master/proposed/phpdoc.md#719-throws) -
It's the default inside the CodeStyle fixer itself. There are some request for configuration but for now it's not possible. (PhpdocOrderFixer - order? PHP-CS-Fixer/PHP-CS-Fixer#1602)
-
The phpDocumentor docs are showing the same examples as the PSR-5 purpose.
I must honestly admit, I don't know better than this is the 'default', interesting to read how you guys think about it. So what will it be?, are we going left or right? In other words, will I undo or implement this fix?
There was a problem hiding this comment.
the PSR-5 standard is still in debate and not accepted. I would suggest following @dnl-blkv suggestion
There was a problem hiding this comment.
So here let's indeed stick to:
@param
@return
@throws
src/Context/ApiContext.php
Outdated
| } else { | ||
| throw new BunqException( | ||
| } | ||
| throw new BunqException( |
There was a problem hiding this comment.
We always separate throws and returns (and also all the control structures) by one newline from the the rest of the code on the same indentation level. Let's please keep to it.
There was a problem hiding this comment.
Newline is missing before throw :(
There was a problem hiding this comment.
Oh wait, but wasn't this reverted?
.gitignore
Outdated
| @@ -77,4 +77,4 @@ vendor/ | |||
| tmp/ | |||
|
|
|||
| .DS_STORE | |||
|
|
|||
| /composer.lock | |||
There was a problem hiding this comment.
composer.lock is already there, so this line can go :)
| "config": { | ||
| "sort-packages": true, | ||
| "platform": { | ||
| "php": "5.6" |
There was a problem hiding this comment.
Why is this needed? Is it 5.6 because it is the lowest platform we support?
There was a problem hiding this comment.
Yes.
- There will be no longer problems as phpstan/phpstan in (7e8fad5). When someone now try to install an packages which has no support for PHP 5.6 they will get an Composer error
composer require phpstan/phpstan
[InvalidArgumentException]
Could not find package phpstan/phpstan at any version matching your PHP version 5.6.0.0
- Now I don't need to change my local environment to PHP 5.6 when I add/update some PHP packages.
src/Context/ApiContext.php
Outdated
| } else { | ||
| throw new BunqException( | ||
| } | ||
| throw new BunqException( |
There was a problem hiding this comment.
Newline is missing before throw :(
src/Context/ApiContext.php
Outdated
| } else { | ||
| throw new BunqException( | ||
| } | ||
| throw new BunqException( |
There was a problem hiding this comment.
Oh wait, but wasn't this reverted?
…:readLine() with incorrect case: readline
…l of rule options the same as before.
- Defined the target platform (PHP 5.6) in Composer.
|
@cafferata Can you also resolve the conflict with |
Done. |
Introduction of Grumphp
Follow the conversation with @dnl-blkv from #3 (comment)
This composer plugin will register some git hooks in the package repository. When somebody commits changes, GrumPHP will run some tests on the committed code. If the tests fail, they won't be able to commit their changes.
Configured tasks
Good to know
With the introduction of the composer package phpstan/phpstan in (7e8fad5), the PHP dependency has been increased to PHP 7.0. How to handle this?