-
Notifications
You must be signed in to change notification settings - Fork 54
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
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thaks for submitting this PR, GrumPhp looks like a great tool.
Please address these issues.
.php_cs
Outdated
], | ||
'linebreak_after_opening_tag' => true, | ||
'no_short_echo_tag' => true, | ||
'no_useless_else' => true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cafferata thanks! lets keep the if/else for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oké cool, did the same with this pull request. :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple comments :)
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So this one indeed should go because we don't want to kill 5.6 compatibility.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's already gone! :-)
src/Context/ApiContext.php
Outdated
* @throws BunqException | ||
* @return string[][] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Newline is missing before throw
:(
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this needed? Is it 5.6 because it is the lowest platform we support?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a couple of comments
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This diff looks weird. It still includes all those changes for if
's and @throw
's ...
src/Context/ApiContext.php
Outdated
} else { | ||
throw new BunqException( | ||
} | ||
throw new BunqException( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Newline is missing before throw
:(
src/Context/ApiContext.php
Outdated
} else { | ||
throw new BunqException( | ||
} | ||
throw new BunqException( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tests passing @andrederoos
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?