-
Notifications
You must be signed in to change notification settings - Fork 137
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
5.x: add property types, return types, declare strict types #384
Conversation
Codecov Report
❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more. @@ Coverage Diff @@
## cake5 #384 +/- ##
============================================
- Coverage 63.77% 63.75% -0.03%
Complexity 682 682
============================================
Files 40 40
Lines 2446 2447 +1
============================================
Hits 1560 1560
- Misses 886 887 +1
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
src/Command/AddCommand.php
Outdated
@@ -42,7 +43,7 @@ public function getOptionParser(): ConsoleOptionParser { | |||
/** | |||
* @param \Cake\Console\Arguments $args Arguments | |||
* @param \Cake\Console\ConsoleIo $io ConsoleIo | |||
* @return int|null|void | |||
* @return null|void |
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.
Well, thats a bit weird ;)
I wonder if we could just set it void then? If we dont plan on returning any actual value
Or we keep the current documented possibility of int still for more BC further down the road.
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.
Actually, looking at others it seems one could also consolidate this to
int|null
as return type and be done with 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.
I would agree if there would be any kind of return $something;
in that method but there is only a simple return
in there.
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.
In that case the change doesnt make sense though, as future changes to it could return an int code.
And we document the possible types, not actual.
Think about extending methods or using methods. Without the proper scope of possible return values known, they would start to only care about the ones actually in there, leaving it open to bugs, and phpstan cannot help to detect these faults.
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.
right, didn't think about that 🤔
@@ -35,7 +36,7 @@ public function initialize(): void { | |||
* Admin center. | |||
* Manage queues from admin backend (without the need to open ssh console window). | |||
* | |||
* @return \Cake\Http\Response|null|void | |||
* @return null|void |
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 still could, maybe we shouldnt remove those then?
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.
well technically that controller action can't because there is no return anywhere in there. It would be different if we'd enforce $this->render();
but we don't
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.
Documented types these days are never about it what it does so much than what it could.
So it documents that at any point you could start returning it, which is plausible and the whole reason we allow this as range either in types or just documentation :)
Adding strict_types is probably a good idea by now for 8.1+ Regarding CS: Maybe we could try to only apply the relevant changes as PR first? |
The current
5.x
version of https://github.com/cakephp/cakephp-codesniffer/tree/5.x requires all properties to have a php native typehint added (if possible). So it automatically fails for e.g.because
now I could have manually added all those typehints to this plugin but this would have been very tedious.
Therefore I
cakephp-codesniffer
5.x versionand here we are with a bunch of
declare(strict_types=1);
added, cleaned up property types, return types and phpdocs.So it seems your cs rules are not as "strict" as CakePHP's are.
I personally would prefer if this plugin would also just go with the rules of
cakephp/cakephp-codesniffer
but this is of course not my decision to make.