Page MenuHomePhabricator

CodeSniffer Generic.Formatting.SpaceAfterCast.NoSpace is incorrect
Closed, ResolvedPublic

Description

CodeSniffer currently tries to enforce a space after a cast[2].

Our documentation states: "When type casting, do not use a space within the cast operator or after the cast"[1]

Please update the CodeSniffer configuration to issue the correct warning.

[1] https://www.mediawiki.org/wiki/Manual:Coding_conventions/PHP
[2] https://integration.wikimedia.org/ci/job/mwext-Translate-phpcs-HEAD/1291/console


Version: wmf-deployment
Severity: enhancement

Details

Reference
bz48450

Event Timeline

bzimport raised the priority of this task from to Low.Nov 22 2014, 1:34 AM
bzimport set Reference to bz48450.
bzimport added a subscriber: Unknown Object (MLST).

Been done with https://gerrit.wikimedia.org/r/#/c/45140/

then reverted with https://gerrit.wikimedia.org/r/#/c/45142/ which actually cause the issue.

The SpaceAfterCast has been readded by Timo in https://gerrit.wikimedia.org/r/#/c/58637/

Timo, can you please explain why this is closed WORKSFORME? See https://gerrit.wikimedia.org/r/#/c/66558/ where https://integration.wikimedia.org/ci/job/mwext-Translate-phpcs-HEAD/1361/console throws an error on https://gerrit.wikimedia.org/r/#/c/66558/2/tag/TranslateDeleteJob.php:

73: ERROR A cast statement must be followed by a single space / Generic.Formatting.SpaceAfterCast.NoSpace
73: $pages = (array)$cache->get( wfMemcKey( 'pt-base', $base ) );

This was a few hours ago.

Yes, that's by design.

Our code had mostly a 50/50 between cast with and without a space afterwards.

There was no explicit rule either way, and we'll have to make changes either way.

I made a judgement call and believe that in our coding style it makes most sense to have a space there. Therefor I removed the rule in phpcs that considered code with a space afterwards to be a violation and instead configured it the other way around.

(In reply to comment #3)

Yes, that's by design.

Our code had mostly a 50/50 between cast with and without a space afterwards.

There was no explicit rule either way, and we'll have to make changes either
way.

I made a judgement call and believe that in our coding style it makes most
sense to have a space there. Therefor I removed the rule in phpcs that
considered code with a space afterwards to be a violation and instead
configured it the other way around.

Our docs say otherwise, and despite your consistent replies here, you have not made an effort to remedy the inconsistencies between the checker and the docs.

We had that debate at the beginning of 2013.

When type casting, do not use a space after the cast:

(int)$foo;

http://www.mediawiki.org/w/index.php?title=Manual:Coding_conventions/PHP&diff=652478&oldid=648613

Later improved with code to not use (aka anything with spaces):

( int )$bar;
( int ) $bar;
( int ) $bar;

http://www.mediawiki.org/w/index.php?title=Manual:Coding_conventions/PHP&diff=674672&oldid=673901

Using the standard from mediawiki/tools/codesniffer.git at commit e1de64e4 , PHPCs processing gives out:

(int)$foo; A cast statement must be followed by a single space
( int )$bar;
A cast statement must be followed by a single space
( int ) $bar; considered valid, should not
( int ) $bar;
considered valid, should not

That is happening since https://gerrit.wikimedia.org/r/#/c/58637/ which did the following change:

  • <rule ref="Generic.Formatting.NoSpaceAfterCast" />

+ <rule ref="Generic.Formatting.SpaceAfterCast" />

We need to revert it again, aka like what I did in https://gerrit.wikimedia.org/r/#/c/45142/ :

  • <rule ref="Generic.Formatting.SpaceAfterCast" />

+ <rule ref="Generic.Formatting.NoSpaceAfterCast" />

Related URL: https://gerrit.wikimedia.org/r/66961 (Gerrit Change I641535209fd477e7d56d8659015bb9c66f10ac68)

Related URL: https://gerrit.wikimedia.org/r/66964 (Gerrit Change Ied10f203af5dcff45893bb0246cb6a0469728cb3)

phpcs no longer warns about (type) casting now.

Timo that bug is a regression, we used to have that sniff in and https://gerrit.wikimedia.org/r/#/c/66961/ reintroduce it.

Change 66961 abandoned by Hashar:
Enable Generic.Formatting.NoSpaceAfterCast

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

I think the status should be "reopened" now, per comment 9. It's pretty confusing that the bot changes bug status to "patch to review" when a patch is abandoned. I'll have to change status twice to get there...

Hmm, can't get to reopened, so will leave this at new... Messed up...

So what is the state here and what do we want?

Per Timo Comment #8 the rule is abandoned so we do not enforce one way or another.

My patch https://gerrit.wikimedia.org/r/#/c/66961/ was to reintroduce one of the style and Timo and I agreed to abandon it (see discussion on Gerrit change).

I guess we can mark this bug as FIXED. The fix being that we abandoned that sniff entirely.

Is this really resolved? Surely if the coding standards say no-space, then it makes sense for phpcs to check that? Anyway, I'm not meaning to bring this ticket back from the other side; I've asked over here: https://www.mediawiki.org/wiki/Manual_talk:Coding_conventions/PHP#Can_we_enforce_no-spaces-after-cast.3F :-)

@Samwilson well the discussion on this ticket went with "lets not enforce any rule for space or non space after casting. The original issue was the PHPCs rule being broken and since we stopped using that solves the task.

I guess the wiki page might be out of date? One sure thing is that the reference is the set of rules defined in mediawiki/tools/codesniffer.git. Feel free to take a look, there is a friendly community around it ;-]

Thanks @hashar, I shall investigate, and perhaps send a patch about which to talk. :-)