Success

User tests: Successful: Unsuccessful:

avatar mbabker
mbabker
16 Jul 2015

PHPCS v2 is generally a bit more strict and a bit better at catching errors. While testing the Joomla Coding Standard with that version of PHPCS, the tracker throws a lot of errors on the v2 code base. All of these are things that aren't really covered in the code standard explicitly but at least make our code consistent with how we handle things. There's still a lot of stuff that we either need to address in the PHPCS sniffs, but this covers a good chunk of the logical stuff.

avatar mbabker mbabker - open - 16 Jul 2015
avatar b2z
b2z - comment - 17 Jul 2015

How do I test it ? :)

avatar photodude
photodude - comment - 18 Jul 2015

@b2z , you could use the current development version if you want to test the PHPCS-2 version of the Joomla coding standards (thanks to @mbabker for the work on fixing the custom sniffs that couldn't be directly replaced with an existing standards rule)
https://github.com/photodude/coding-standards/tree/phpcs-2

avatar b2z
b2z - comment - 18 Jul 2015

@photodude thanks! @mbabker does it mean that we are now sticking to PHPCS-2?

avatar mbabker
mbabker - comment - 18 Jul 2015

Not yet. Eventually when we get the project's ruleset stabilized on
PHPCS-2 we'll migrate up to it (as should the rest of the project's code
repos).

On Sat, Jul 18, 2015 at 12:20 AM, Dmitry Rekun notifications@github.com
wrote:

@photodude https://github.com/photodude thanks! @mbabker
https://github.com/mbabker does it mean that we are now sticking to
PHPCS-2?


Reply to this email directly or view it on GitHub
#680 (comment).

avatar elkuku
elkuku - comment - 18 Jul 2015

May I just bark in here to confess that I definitely LOVE multiline concats and terneries?

I mean we should adapt the standards to our needs not the other way around...

avatar mbabker
mbabker - comment - 18 Jul 2015

Some of it is definitely needing to be adapted still.

On Saturday, July 18, 2015, Nikolai Plath notifications@github.com wrote:

May I just bark in here to confess that I definitely LOVE multiline
concats and terneries?

I mean we should adapt the standards to our needs not the other way
around...


Reply to this email directly or view it on GitHub
#680 (comment).

avatar photodude
photodude - comment - 18 Jul 2015

At this point most of the work has been in getting a functional version of the joomla coding standards and reducing the number of custom Sniffs and replacing them with existing standards included in PHPCS. Still lots to do to get the standards matching with all of the points in the written standards.

The PHPCS2 version of the joomla coding standards will definitely be a huge help in enforcing the written coding style since PHPCS 2 includes fixer methods to automatically correct coding style deviations.

avatar elkuku
elkuku - comment - 18 Jul 2015

@photodude they say that german people tend to talk "rude". I don't know why... sorry if my words had a negative tone, that was not my intention.

Adapting our coding standards to V2 of phpcs is a long outstanding task (at least since it has become stable and being installed by default).
Great work here - BIG THANKS!
(btw I was heavily involved in the creation of our V1 standards and most of the custom sniffs were adapted or created by me).

On the other hand I don't think that we should change beautiful looking code because a standard says so.
No this is not a discussion about code style, but I think we should adapt the sniffs to permit currently used formattings. If you want me to get my hands dirty on the code I can definitely do so (I guess). :wink:

avatar photodude
photodude - comment - 18 Jul 2015

@elkuku it's all good. Nothing seemed "rude" to me. I think there are nuances lost in writing that can make a simple statement seem to have a different tone than was actually intended.

It's all volunteer work, you are more than welcome to help out. As they say in the movie Robots, "See a need, fill a need".

avatar b2z
b2z - comment - 19 Jul 2015

Ok we are not sticking to v2 then I am continue to code with v1? I cannot switch to different sniffers in my IDE :( And what to do with this PR then? The transition path it not clear to me.

avatar mbabker
mbabker - comment - 19 Jul 2015

Thinking about this on my drive home from the airport, specifically the
ternery operators being multiline, as that is essentially an if/else
statement, I would almost suggest our code standard disallows multiline
ternery operations. It's just my opinion, but when those start becoming
multiline statements, it seems to make sense to me to write it as a proper
if/else structure.

On Saturday, July 18, 2015, Nikolai Plath notifications@github.com wrote:

May I just bark in here to confess that I definitely LOVE multiline
concats and terneries?

I mean we should adapt the standards to our needs not the other way
around...


Reply to this email directly or view it on GitHub
#680 (comment).

avatar photodude
photodude - comment - 19 Jul 2015

I think multi-line ternary operators could have a benefit, if they improve the readability of the code. I would definitely suggest disallowing nested or stacked ternary operators as those situations just create a mess for code readability and maintenance.

With that said; based on some things I have recently read, I question even allowing ternary operators. Or at least strongly discouraging them.

In general I think if a code standard or code style guidelines achieve the following then they are probably something that should be considered for adoption.

  • Does it improve code consistency?
  • Does it improve code readability?
  • Does it improve future maintainability?
  • Does it improve code portability?
  • Does it improve code performance?
  • Does it improve code security?

So applying those questions to ternary operators:

  • They are a deviation from standard if/else, thus creating a question of consistency.
  • They generally reduce readability
  • They generally reduce maintainability
  • They can reduce performance (since ternary operators do a copy operation, larger data sets will cause more performance issues)
  • As for portability, and security I don't know of any pros or cons.

I would also say that there are places where short hand code is discouraged or not allowed (PHP tags for instance). So this maybe another area where the short hand operators may not be the best choice.

avatar b2z
b2z - comment - 20 Jul 2015

Guys so what should I do? Continue to code JTracker with v1 or switch to v2?

avatar mbabker
mbabker - comment - 20 Jul 2015

Keep on with v1 for now. The v2 standard isn't stabilized yet.

avatar b2z
b2z - comment - 20 Jul 2015

ok thanks :)

avatar mbabker
mbabker - comment - 22 Aug 2015

Closing for now. Will keep testing the PHPCS changes and come back when the standard is more stable :wink:

avatar mbabker mbabker - change - 22 Aug 2015
Status New Closed
Closed_Date 0000-00-00 00:00:00 2015-08-22 17:08:20
Closed_By mbabker
avatar mbabker mbabker - close - 22 Aug 2015
avatar mbabker mbabker - close - 22 Aug 2015
avatar mbabker mbabker - head_ref_deleted - 22 Aug 2015
avatar elkuku
elkuku - comment - 23 Aug 2015

Too bad. Actually I liked most of the changes proposed here... Hope you come back soon :wink:

avatar mbabker
mbabker - comment - 23 Aug 2015

Still have the branch locally and still using this as my test bed for the PHPCS 2 standard. Just depends how much time I can push into helping stabilize that.

avatar photodude
photodude - comment - 25 Aug 2015

@elkuku Work is still active on the PHPCS2-joomla code standard. This PR was just related to testing of the in progress work.

Follow this PR in the Code standard to keep tabs on the work towards a stable release.
joomla/coding-standards#109

avatar elkuku
elkuku - comment - 12 Sep 2015

I was just reading about the possibility of chaining null coalesce operators in ternaries - I hope our coding standards will allow them multi lined :wink:

avatar vess
vess - comment - 21 Sep 2015

@elkuku I agree with you that multi lines should be supported here, however, Squiz.WhiteSpace.OperatorSpacing.NoSpaceBefore throws an error here (multi and single line), seems that the operator isn't supported yet, but I guess it will be in near future.

Right now there are still some issues left, so if you have some time to spend, get your hands dirty please.

Add a Comment

Login with GitHub to post a comment