-
Notifications
You must be signed in to change notification settings - Fork 10
Use Slevomat Coding Standard method typehinting #83
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
Conversation
composer.json
Outdated
@@ -28,6 +28,7 @@ | |||
"squizlabs/php_codesniffer": "^3.2.3", | |||
"escapestudios/symfony2-coding-standard": "^3.0", | |||
"dealerdirect/phpcodesniffer-composer-installer": "^0.4.3", | |||
"php": ">=5.6.0" | |||
"php": ">=5.6.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.
As Slevomat requires PHP 7.1, we could also bump this here.
Using Slevomat means to drop PHP 5. Symfony 3.4 is is requires at least 5.5.9. So this PR would be a breaking change. Or shall we create a new branch to keep compat to 3.4 for 5.5.9? |
@xalopp Please see GH-84 for a separate PHP 7.1 change. I'd suggest to merge GH-85 first and then branch off 2.x. But to be honest, personally I don't see any relationship between our CS and Symfony's PHP requirements. Even now we do require 5.6, which is >5.5.9. Bringing in this new sniff here would require a major version bump anyway. |
1416813
to
d15bf7f
Compare
Codecov Report
@@ Coverage Diff @@
## master #83 +/- ##
=========================================
Coverage 98.46% 98.46%
Complexity 124 124
=========================================
Files 6 6
Lines 522 522
=========================================
Hits 514 514
Misses 8 8 Continue to review full report at Codecov.
|
d15bf7f
to
4947d2b
Compare
README.md
Outdated
@@ -35,7 +35,10 @@ The MO4 Coding Standard is an extension of the [Symfony Coding Standard](http:// | |||
* There must be at least one space around operators, and (except for aligning multiline statements) at most one, see the | |||
[respective Squizlabs Sniff](https://github.com/squizlabs/PHP_CodeSniffer/wiki/Customisable-Sniff-Properties#squizwhitespaceoperatorspacing) | |||
we imported with `ignoreNewlines = false` | |||
* Single quotes must be used instead of double quoutes, where possible. | |||
* Single quotes must be used instead of double quotes, where possible. | |||
* all method requires a return type; use `void` if the method returns nothing. |
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.
"all methods require"
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.
And for consistency reasons with the bullet points above I'd vote for uppercase first :)
README.md
Outdated
* Single quotes must be used instead of double quoutes, where possible. | ||
* Single quotes must be used instead of double quotes, where possible. | ||
* all method requires a return type; use `void` if the method returns nothing. | ||
* all method parameters that defaults to null must be marked as nullable with the `?` symbol. |
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.
"that default" and maybe use backticks for the null
I ran this against a small modern codebase and the results are pretty great 👍 |
@xalopp could you please also add |
4947d2b
to
d87604e
Compare
No description provided.