237: scrutinizer clean up #307
Conversation
|
I'm not sure which coding standard you're using, but there are rules being updated that include whitespace and some that don't e.g.: $rules->name_should_not_be('empty');vs $rules->status_should_not_be( 'empty' );Maybe not strictly picked up in Scrutinizer, but now would be the ideal time to fix them up for consistency. |
|
In those cases I was just replacing the variable names, the rest was left as originally done. There's a case to be made to go through the entire codebase and fix up the coding layout. |
If this project follows the same principles as WP core, then PRs just for the sake of code formatting is generally frowned upon, hence the case for doing this now when the lines of code are already being touched. |
| @@ -7,7 +7,7 @@ class GP_CLI_Translation_Set extends WP_CLI_Command { | |||
| * @param string $project Project path | |||
| * @param string $locale Locale slug | |||
| * @param string $set Set slug | |||
| * @return GP_Translation_Set|WP_Error Translation set if available, error otherwise. | |||
| * @return object Translation set (GP_Translation_Set class) if available, error (WP_Error class) otherwise. Note, PHPDoc doesn't know about WP_Error so we can explicitly define it in the return types. | |||
There was a problem hiding this comment.
I don't accept that reason. We don't write code for tools, we're writing code for users and developers.
There was a problem hiding this comment.
I'll revert it, but slightly disagree with the above. Doc blocks serve a dual purpose, make it easy for developers to understand the code and let tools work with the codebase in a consistent manner.
The above change accomplishes both goals where as the original format only accomplished one of them.
There was a problem hiding this comment.
and let tools work with the codebase in a consistent manner.
True, but it's a plugin for WordPress. Maybe there is a way to include WordPress as a library? Another example is WP_Query.
There was a problem hiding this comment.
Not sure, other people must have run in to this, we should look in to it more.
There was a problem hiding this comment.
Do you have a link handy for the scrutinizer issue?
There was a problem hiding this comment.
Should be these three if I recall correctly
toolstack
force-pushed
the
237-scrutinizer-clean-up
branch
2 times, most recently
from
81f84e7
to
7b7bdc1
Compare
toolstack
force-pushed
the
237-scrutinizer-clean-up
branch
from
7b7bdc1
to
4ba6ef1
Compare
|
Reviewing. |
toolstack
force-pushed
the
237-scrutinizer-clean-up
branch
from
a5d4cbd
to
d0cba01
Compare
ocean90
force-pushed
the
237-scrutinizer-clean-up
branch
from
f8e793a
to
dd9a8d3
Compare
…e a more descriptive variable name.
…he magic methods.
…he magic methods.
…ins screen. Reduces code duplication.
ocean90
force-pushed
the
237-scrutinizer-clean-up
branch
from
7417a83
to
31bff5b
Compare
Additional cleanups based on scrutinizer, part of #237.