Page MenuHomePhabricator

Fix braces issues in MediaWiki core code
Closed, ResolvedPublic

Description

Includes:

  1. Opening brace must be the last content on the line (Generic.Functions.OpeningFunctionBraceKernighanRitchie.ContentAfterBrace)
  2. Closing brace must be on a line by itself (Squiz.WhiteSpace.ScopeClosingBrace.ContentBefore)
  3. Expected 1 space after closing parenthesis

Event Timeline

polybuildr raised the priority of this task from to Needs Triage.
polybuildr updated the task description. (Show Details)
polybuildr added subscribers: Krenair, Addshore, Legoktm and 3 others.
polybuildr updated the task description. (Show Details)Jun 17 2015, 1:26 PM
polybuildr set Security to None.

I'll upload a patch set to fix these two very soon. However, I'm not sure whether my corrections are the best way to go about it.

I've changed some

function wfFoo() { // Bad function
    $bar = 5;
    ...

to

function wfFoo() {
    // Bad function

    $bar = 5;
    ...

and I've broken some empty function definitions from

function wfBC() {}

to

function wfBC() {
}

Comments?

Change 218889 had a related patch set uploaded (by Polybuildr):
Fix braces code style

https://gerrit.wikimedia.org/r/218889

Change 218889 merged by jenkins-bot:
Fix braces code style

https://gerrit.wikimedia.org/r/218889

The patch has been merged, but I'm still not sure if it was the best way. Leaving this task open for now, waiting for responses.

Seems fine to me.

polybuildr closed this task as Resolved.Jun 17 2015, 6:09 PM
polybuildr removed a project: Patch-For-Review.
function wfBC() {
}

LGTM

Nikerabbit reopened this task as Open.Jun 18 2015, 6:46 AM
Nikerabbit added a subscriber: Nikerabbit.

I dispute the sniff "Closing brace must be on a line by itself" as it prevents doing one-line anonymous functions, such as function ( $file ) use ( $dir ) { return "$dir/$file"; } in https://gerrit.wikimedia.org/r/#/c/217320/1..2/MessageGroups.php. I also don't like forcing body-less functions to two lines.

hashar removed a subscriber: hashar.Jun 18 2015, 12:55 PM

@Legoktm, @bd808, any responses to @Nikerabbit's comment?

I dispute the sniff "Closing brace must be on a line by itself" as it prevents doing one-line anonymous functions, such as function ( $file ) use ( $dir ) { return "$dir/$file"; } in https://gerrit.wikimedia.org/r/#/c/217320/1..2/MessageGroups.php. I also don't like forcing body-less functions to two lines.

Single line anonymous functions sound like a contradiction of existing style guide information:

Do not write "blocks" as a single-line. They reduce the readability of the code by moving important statements away from the left margin, where the reader is looking for them. Remember that making code shorter doesn't make it simpler. The goal of coding style is to communicate effectively with humans, not to fit computer-readable text into a small space. -- Manual:Coding_conventions#Braceless_control_structures via Manual:Coding_conventions/PHP#Brace_placement

In general, coding conventions for multi-developer projects (and especially ones with automated validation) are a compromise. Nobody will like all of the rules. I personally find the extra whitespace inside parens rule to be useless and annoying, but it's the project convention so I follow it. As long as the enforced rules that are being introduced don't require large scale change of the codebase (i.e. they fit the large majority of the existing manually enforced code style) I don't see a problem.

polybuildr closed this task as Resolved.Jun 25 2015, 9:44 AM

@Nikerabbit, I think @bd808's comment makes sense. I'm closing this task since its scope was only to make core pass the sniff, not discuss the sniff itself, but feel free to make a new task to dispute the sniff, of course.

Ricordisamoa edited subscribers, added: Ricordisamoa; removed: gerritbot.Sep 25 2015, 9:53 PM