SilverStripe Coding Conventions and PHPCodesniffer

107 views
Skip to first unread message

Stig Lindqvist

unread,
Sep 21, 2012, 1:47:26 AM9/21/12
to silverst...@googlegroups.com
Hi all,

I've been trying to put together a ruleset for SilverStripe coding
conventions and would like to announce my ideas here.

I rather have an automated way of checking my code than trying to
remember to implement it. I and many other really likes scripts
telling us that we're not using the correct syntax, implementing bad
patterns and correcting us.

During the years of SilverStripe have been developed it has seen a lot
of different coders and styles being pushed into it.

I would greatly appreciate any feedback and collaboration on this. I
might even convince the SS head honchos to make it official and we
might expand / clean up the current conventions at
http://doc.silverstripe.org/framework/en/trunk/misc/coding-conventions..

github repo: https://github.com/stojg/silverstripe-codesniffer/

Cheers,
Stig
Automation freak

Ingo Schommer

unread,
Sep 21, 2012, 4:31:00 AM9/21/12
to silverst...@googlegroups.com
FREAKING AWESOME! I remember you showing me this when you were
still in Sweden, and I've had a todo item for "port Stig's codesniffer to new conventions" ever since heh.

We could hook into Travis, which would be the most visible automation.
But from what I can tell, we can't run the sniffer against diffs.
So for now, even if your changes are adhereing to the conventions,
but the file you changed is not, the sniffer will fail, right?

If that's the case, we need to fix up core first, which won't be a trivial exercise.
There's a project which might help with the basic bits: https://github.com/fabpot/PHP-CS-Fixer
Its written by the Symfony lead dev, and applied to Symfony code, so should be fairly solid.

That won't fix stuff like wrong class/method naming conventions of course,
of which we have plenty - those will be a slow deprecation cycle to fix. 
For any automation, we'd need to suppress those errors to keep the output meaningful.
Overall, I think the most useful mid-term application would be a diff of the sniffer error output,
which should be doable in CI (at least for TeamCity, not sure for Travis in terms of persistence).

Ingo

Sam Minnée

unread,
Sep 21, 2012, 6:42:55 PM9/21/12
to silverst...@googlegroups.com, silverst...@googlegroups.com
If we can add a whitelist of any incompatible method names, then we can still include the test and use to ensure that no new names are introduced. Alternatively, we could only allow incompatible method names if they are @deprecated, but I don't know how hard that is with the sniffing framework.

Again: our goal should be to have an automated check for every potential objection to a pull request that we can push off human reviewers' plates, so I also support getting this into the CI build.
--
You received this message because you are subscribed to the Google Groups "SilverStripe Core Development" group.
To view this discussion on the web visit https://groups.google.com/d/msg/silverstripe-dev/-/gi5MC2FrzjAJ.
To post to this group, send email to silverst...@googlegroups.com.
To unsubscribe from this group, send email to silverstripe-d...@googlegroups.com.
For more options, visit this group at http://groups.google.com/group/silverstripe-dev?hl=en.

Che Van Lawrence

unread,
Sep 23, 2012, 4:42:31 PM9/23/12
to silverst...@googlegroups.com
That sounds awesome Stig 

+1

Stig Lindqvist

unread,
Sep 23, 2012, 5:54:12 PM9/23/12
to silverst...@googlegroups.com


On Friday, 21 September 2012 20:31:01 UTC+12, Ingo Schommer wrote:

We could hook into Travis, which would be the most visible automation.
But from what I can tell, we can't run the sniffer against diffs.
So for now, even if your changes are adhereing to the conventions,
but the file you changed is not, the sniffer will fail, right?

PHPCodesniffer is using the lexer, so it would need a proper PHP file, it can't work on partial code snippets. We might need to extract the filenames only from a diff and run the sniffer on those files. 

If that's the case, we need to fix up core first, which won't be a trivial exercise.
There's a project which might help with the basic bits: https://github.com/fabpot/PHP-CS-Fixer
Its written by the Symfony lead dev, and applied to Symfony code, so should be fairly solid.

That seems much better than the now dormant PHP beutifier. Looked briefly at it, but I have no idea how big effort it would be to get something going.
 

That won't fix stuff like wrong class/method naming conventions of course,
of which we have plenty - those will be a slow deprecation cycle to fix. 

For any automation, we'd need to suppress those errors to keep the output meaningful.
Overall, I think the most useful mid-term application would be a diff of the sniffer error output,
which should be doable in CI (at least for TeamCity, not sure for Travis in terms of persistence).


I've been looking at a code quality tool called Sonar source that I've got working with a project. It's currently only for a small internal project, but I can provide a link to what kind of metric it can display.
 

Stig Lindqvist

unread,
Sep 23, 2012, 6:01:41 PM9/23/12
to silverst...@googlegroups.com

On Saturday, 22 September 2012 10:43:01 UTC+12, Sam Minnée wrote:
If we can add a whitelist of any incompatible method names, then we can still include the test and use to ensure that no new names are introduced. Alternatively, we could only allow incompatible method names if they are @deprecated, but I don't know how hard that is with the sniffing framework.

Ages back I did some custom sniffs to check for method names. Since a lot of method naming depends on the usage it's a bit hard to fully automate it. Put it's possible to check that static methods are lower case named. Currently the ruleset doesn't check for it.

Checking for annotations, tricky, but it's as it's possible to make our own sniffs it should be possible. Would probably take some effort of getting it going.
 

Again: our goal should be to have an automated check for every potential objection to a pull request that we can push off human reviewers' plates, so I also support getting this into the CI build.

It might also safe guard against common code smells such as cyclomatic complexity, god methods etc. 

Dan Rye

unread,
Sep 23, 2012, 6:45:28 PM9/23/12
to silverst...@googlegroups.com
+1 for Sonar

To view this discussion on the web visit https://groups.google.com/d/msg/silverstripe-dev/-/Faaz8pV1R-4J.

Sam Minnée

unread,
Sep 23, 2012, 6:56:32 PM9/23/12
to silverst...@googlegroups.com, silverst...@googlegroups.com
I want to throw a bit of caution: like blue cheese, a code smell shouldn't necessarily break the build. I'd prefer to start small and build up the test suite incrementally.

Let's focus on getting the simplest possible test into our test suite, and then look to grow it.

Sam Minnée

unread,
Sep 28, 2012, 1:03:17 AM9/28/12
to silverst...@googlegroups.com
OK, I've added a minimal first step in this direction here:



On Monday, 24 September 2012 10:56:51 UTC+12, Sam Minnée wrote:
I want to throw a bit of caution: like blue cheese, a code smell shouldn't necessarily break the build. I'd prefer to start small and build up the test suite incrementally.

Let's focus on getting the simplest possible test into our test suite, and then look to grow it.

Reply all
Reply to author
Forward
0 new messages