Coding style: Language construct spacing - Wikitech-l

9 May 2013

Hi,

Since there appears to have been a little bit of trivia around fixing
these phpcs warnings, I'll open a thread instead.

Both in javascript and PHP there are various keywords that can be used
as if they are functions. In my opinion this is a misuse of the
language and only causes confusion.

I'm referring to code like this (both javascript and php):

    delete( mw.legacy );

    new( mw.Title );

    typeof( mw );

    echo( $foo . $bar );

    print( $foo . $bar );

    return( $foo . $bar );

… and, wait for it.. 

    require_once( $foo . $bar );

I think most experienced javascript developers know by now that using
"delete" or "new" like it is a function is just silly and looks like
you don't know what you're doing.

To give a bit of background, here's why these work at all (they aren't
implemented both keywords and functions, just keywords). Though I'm
sure the implementation details differ between PHP and javascript, the
end result is the same: Keywords are given expressions which are then
evaluated and the result is used as value. Since expressions can be
wrapped in parenthesis for readability (or logic grouping), and since
whitespace is insignificant to the interpreter, it is possible to do
`return("test")`, which really is just `return ("test")` and
eventually `return "test"`.

I'm obviously biased, but I think the same goes for "require_once"
(and "include", "require" etc.). Right now this is causing quite a
few
warnings in our php-checkstyle report.

I didn't disable that rule because it appears (using our code base as
status quo) that we do want this. There's 0 warnings I could find in
our code base that violate this, except for when the keyword is
include|require(_once)?

The check style sniffer does not (and imho should not) have a separate
rule per keyword. Either you use constructs like this, or you don't.

But let's not have some weird exception just because someone didn't
understand it[1] and we all copied it and want to keep it for no
rational reason.

Because that would mean we have to either hack the sniffer to exclude
this somehow, or we need to disable the rule, thus not catching the
ones we do use.

See pending change in gerrit that does a quick pass of (most of) these
in mediawiki/core:

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

-- Krinkle

[1] Or whatever the reason is the author originally wrote it like
this. Perhaps PHP was different back then, or perhaps there was a
different coding style.