-
Notifications
You must be signed in to change notification settings - Fork 614
PHP-CS-Fixer Symfony #1628
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
PHP-CS-Fixer Symfony #1628
Conversation
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.
@carlos-granados this looks generally great to me. A couple of personal opinions on style that I'm happy to be overruled on but other than that completely agree with the rules you've applied.
Only if you were going to make any changes to the rules it might be worth splitting the "remove @inheritdoc / unnecessary PHPdoc params" commit to a separate PR first. That was the one github struggled to render for me, and it seemed like it then upset their logic for identifying which files could safely be marked as "Viewed" when looking at a single commit, which made it a little slower to review the subsequent ones.
e9c90ce
to
3e91cc0
Compare
@acoulton I rebased this code and added some changes
|
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.
@carlos-granados this is great, thanks. My CPU definitely screamed less at having the phpdoc changes in two commits 😆 thanks for overriding the alignment rule.
Agreed on the !
operator. Also on the outputDecorated
: thinking about it we're not calling those setters in strict type mode anyway, and I can't imagine people are calling it with something that can't at least cast to a bool so it should be fine.
3e91cc0
to
8bd5f59
Compare
Updates our code to use the Symfony coding style
Replaces the default value for 5 rules:
concat_space
In the Symfony standard all spacing around the concat (.
) operator is removed. I believe this just makes the code much less readablephpdoc_separation
This is just modified so that it won't insert blank lines between Given, When and Then annotationsglobal_namespace_import
Symfony does not use imports for Classes and functions in the global namespace (for example it will use\DateTime
instead of adding ause DateTime;
statement). I think this goes against what we have been enforcing in our projectsingle_line_throw
With the Symfony setting all throw statements use a single line but if we do this we end with some very long linesyoda_style
Symfony uses yoda comparisons but in these days of static analysers this is no longer really useful and it just makes the code more unreadablePlease let me know if you disagree with any of these exceptions to the Symfony standard
There are a huge number of files changed. As in previous CS pull requests I have separated the changes in different commits with logically related rules to make this easier to review. Some of these commits are themselves quite big even if they result from the application of a single rule, but in those the changes will be all very similar so I am hoping they will be easy to review. Let me know if you would prefer me to split these large commits into smaller ones.
As in other CS pull requests, once everyone is OK with the changes, I will add another commit with the git blame ignore list of commits that need to be ignored