Another round of code formatting?

[totten]

A couple years ago, we adopted a relaxed subset of the Drupal coding standard ( http://wiki.civicrm.org/confluence/display/CRMDOC/PHP+Code+and+Inline+Documentation ). At the time, we did a big, automated reformatting. Generally, I think the code formatting is more consistent today than originally, but it still feels like one comes across inconsistent formatting pretty frequently. This is probably some natural "drift" which arises because the standards aren't strongly enforced.

I've been playing with "phpcs"/"phpcbf" (https://github.com/squizlabs/PHP_CodeSniffer) and Drupal "coder" 2.x (https://www.drupal.org/project/coder). phpcs is pretty nice because it (a) supports many different styles, (b) works on the command-line, and (c) integrates with multiple IDEs. Coder 2.x is basically a set of config files for phpcs, and https://github.com/civicrm/coder is a relaxed version of those config files.

The automated cleanup tool (phpcbf) suggests changes to ~2% of the codebase (~12k/~530k lines of code in api/, CRM/, and Civi/). After the automated formatting, I estimate (based on some spot-checking) that ~5% of the codebase (~26 ksloc) will still have some kind of style issue. (Some style issues -- such as missing docblocks -- can't be corrected automatically by phpcbf.) (Note: A net 7% of deviations may not sound so bad, but I think it's actually quite perceptible. One probably needs to reach >99% to make the code "feel" consistent.)

So here's my proposal:

• Add phpcs, coder, and jshint to buildkit.
• Perform the automated cleanup.
• Spend several hours alternately (a) relaxing the style rules further, (b) running some regex/throw-away scripts, and (c) manually addressing problems to get the remaining code to a passing point.
• Add a command "bin/civilint" which executes "phpcs" and "jshint" (http://wiki.civicrm.org/confluence/display/CRMDOC/Javascript+Reference) with the appropriate standards. (The script should work out to 5-20 SLOC.)
• Encourage developers to run "bin/civilint" on their workstations before submitting PRs.
• Update Jenkins to run "bin/civilint" whenever testing PRs on master/v4.6+.

These steps would not produce perfect style (e.g. some rules are relaxed), but it achieves a "steady-state" where we have and can maintain a certain level of consistency. At some point in the future, we could re-tighten the style rules, do further cleanup, and possibly add more checks to "bin/civilint".

A significant consideration of any broad style cleanup is that it increases the probability of merge-conflicts between versions. To minimize this, I think the ideal timing for the next automated cleanup is right around the release of 4.6.alpha1 (scheduled for next week). That point in time has a few attributes:

• It comes long enough after the release of 4.5 that we shouldn't have a high volume of changes to merge-forward from 4.5 to 4.6.
• It comes late enough in the 4.6 cycle that the big/long PRs (e.g. recurring, CiviMail) have already been merged.
• It comes early enough in the 4.6 cycle that most/all 4.6.x releases will have the same consistency.
• It comes before branching for the next release.
• It's not during a sprint (where there's a high flow of PRs).

[Coleman Watts]

+1 from me. I've recently done a big round of docblock cleanups in the master branch, so not sure what % that's at right now but it's a lot better than it was.

Curious if your cleanup would include lowercasing TRUE FALSE and NULL. I've heard a couple voices pushing for lowercase rather than uppercase and I would +1 them because

• They are not constants
• It's more consistent with other languages like js
• Just because Drupal does something doesn't make it right

[totten]

Re: cleanups -- Cool.

Re: TRUE/FALSE/NULL -- I personally agree with you... because Java/JS/C++/PSR-2 all use lowercase. In fact, I personally disagree with just about everything in the Drupal style guidelines. (Two-spaces? I prefer tabs. Newlines before catch{} or elseif{}? Not for me. under_scores? No, camelCase is better. DrupalClassNamesLikeThis? No, I\Like\NameSpaced\Classes. Formatting of @param? That depends -- it's not black/white.) Even at a meta/community-level, I'd prefer following the PSR-2 community over the Drupal community. But so it goes with any style guidelines - there's a ton of arbitrary decisions, and you can always find people to disagree with them. The main value of a style-guideline comes from consistency and network-effects.

The "pitch" for embracing Drupal style was to make an easier transition for the large constituency of developers who come to Civi from the Drupal community. IMHO, we shouldn't amend Drupal style unless (a) it "relaxes" the Drupal guidelines (ie an incoming Drupal dev writing in Drupal-compliant style would necessarily be Civi-compliant) and (b) it serves a pragmatic purpose (eg preserving technical compatibility or getting to a steady-state more quickly).

[Coleman Watts]

Yea, I actually had heard a rumor that Drupal was switching its stance on uppercasing those but a google search on that turns up nothing so I guess it was just a rumor. Nevermind we'll stick with Drupal's dumb guidelines

[Coleman Watts]

IMO the biggest code formatting problem we have right now is with our tpls. I would categorize our php code formatting as currently "pretty good" while I'd call the tpls "atrocious"
PhpStorm does a nice job of auto-fixing the whitespace problems, which makes them way better (not perfect but way better). Not sure if there's a way to do that in bulk...

[totten]

Tools designed to format HTML might do an OK job with Smarty. The main question mark is what happens to indentation around Smarty flow-control directives, e.g.

Code: [Select]

<!-- A -->
<div>
  {if ...}
    Hello
  {/if}
</div>

<!-- B -->
<div>
  {if ...}
  Hello
  {/if}
</div>

http://en.wikipedia.org/wiki/HTML_Tidy comes to mind, although it's written in C, so it's a little tricky to distribute/require in a cross-platform fashion. You could try it and see how well it works.

[totten]

https://github.com/civicrm/civicrm-buildkit/pull/120

I've updated buildkit to include phpcs, coder, jshint, and the new script "civilint". Note that you can run "civilint" without any arguments, and it will check the style on any uncommitted files.

To get the update:
 * Make sure you have NodeJS. (This is required for jshint and 4.6 dev -- https://civicrm.org/blogs/totten/new-46-dev-composer-and-nodejs )
 * Update buildkit ("cd buildkit; git pull; ./bin/civi-download-tools"

[totten]

Filed an issue: https://issues.civicrm.org/jira/browse/INFRA-132

[JohnFF]

(I can't be the only one who read the title of this forum thread and thought "huzzah!")

[totten]

The automated cleanup is reaching its limit -- the remaining issues are leftovers (*after* running phpcbf), and further automated cleanup requires custom scripting. For issues that affect 10-200 lines of code, it's not really worth the effort to write such scripts.

Rather, I've had good experience using scripts like these: http://think.hm/tmp/cleanup/ Basically, you take one of the style issues (e.g. "Drupal.Commenting.ClassComment.Missing") and run a script (e.g. "bash cleanp-Drupal.Commenting.ClassComment.Missing.sh.txt vi"). The script will open a text-editor (vi or joe) on the offending line so that you can read/edit/save. It goes fairly quick (for me, 2-5sec per line) this way because you know exactly what to look for, and it's safer than a naive regex. (Yay humans! We're more intelligent than regex!)

I think we can get through these if we break up the work a bit. There's ~2600 lines to touch in this range -- or ~3.5 hours of (continuous) editing. If we can get 5-10 people to each do 15-30 min of cleanup, then we'd clear through it.

What do you think - could we gather a couple folks and close out this backlog? If we close the backlog, then we can turn on an automated enforcer in Jenkins/Github - and keep our code clean going forward.

[Eileen]

You can also use phpstorm inspect code - which is what I used to add / a load of comment blocks last year (ie more than 9 days ago)

[Eileen]

Here is what it looks like if you run the phpdoc inspection per the pic
https://github.com/civicrm/civicrm-core/pull/4885

[Eileen]

NB the internal is what it does with something it things is invalid - ie.

@param array<String,SimpleXMLElement> $xml

doesn't 'pass'

but
@param array|String|SimpleXMLElement $xml

does

- * @param array<String,SimpleXMLElement> $xml
+ * @param null $allCaseTypes
+ * @param array $xml
+ * @internal param $array <String,SimpleXMLElement> $xml

[totten]

The PR looks generally like an improvement. It's cool when PHPStorm guesses the types correctly. I am bit nervous about the @internal gunk and about badly guessed types ("@param null $foo").

Regarding "@param array<String,SimpleXMLElement>", I've found that the best way to appease phpcs/Drupal-style is to move the array details into the body of the description, e.g.

Code: [Select]

/**
 * @param array $xml
 *   Array<string,SimpleXMLElement> Case-type definitions keyed by case-type name.
 */

/**
 * @param array $xml
 *   Array(string $caseTypeName => SimpleXMLElement $defn).
 */

(Aside: For the record, "array|String|SimpleXMLElement" means something very different from "array<String,SimpleXMLElement>". I think Eileen knows this, but forums get indexed by Google, and other readers might interpret the last comment as equating them.)

For the moment, https://github.com/civicrm/coder basically turns off the most significant phpcs warnings about docblocks (e.g. for missing types and missing comments). There's simply too many to cleanup right now, and it takes some judgment to do them properly. Even with PHPStorm "Inspect Code", it leaves a bunch of blanks and incorrect NULL types.

I really want to get to a point where we can enforce some meaningful standards (even if they don't have they're not as meaty about docblocks). When we have a baseline, it'll be possible to ramp-up the enforcement incrementally over time. But right now we can't enforce anything because we've got a backlog.

[Coleman Watts]

Eileen: not that I don't appreciate the big phpStorm auto-cleanup you committed last year (I do), but FYI I spent a couple days manually cleaning up docblocks afterwards (ok it wasn't all manual, I used lots of regexes when I could get away with it) and fixing stuff like:

• If the original docblock had started with /* instead of /** then phpStorm didn't see it and created a second one.
• Understandably, it was bad at coping with misspelled @params (including something like @param contactId missing the $ at the beginning of the var name)
• As Tim noted, the type guessing is fairly bad. I did some mass regex replaces to fix some of them like changing any param with a name like $*id from type null (obviously wrong) to integer (probably right)
• The @internal params were kind of a mess, but at least they were easy to find. I grepped on @internal and then manually fixed whatever had gone wrong with the auto-cleanup

Ok I didn't stop at just cleaning up after phpStorm I also used my find/replace powers to get rid of some inane comments (like "// end of function" at the end of a function, or starting function descriptions with "This is a function...")