Add a sniff for method operator followed by newline
Open, MediumPublic
Actions

Assigned To

None

Authored By

	DannyS712
	May 20 2020, 4:42 AM

Project Tags

Referenced Files

None

Subscribers

Description

https://www.mediawiki.org/wiki/Manual:Coding_conventions#Line_continuation says
The method operator should always be put at the beginning of the next line.

$this->getMockBuilder( Message::class )->setMethods( [ 'fetchMessage' ] )
    ->disableOriginalConstructor()
    ->getMock();

Example: I came across the following in the Wikispeech extension

$config = MediaWikiServices::getInstance()->
	getConfigFactory()->
	makeConfig( 'wikispeech' );

While this works, the coding convention should be enforced by a sniff.

A basic code search (https://codesearch.wmflabs.org/search/?q=-%3E%24&i=nope&files=%5C.php%24&repos=) reveals 64 violations in core

Event Timeline

DannyS712 created this task.May 20 2020, 4:42 AM

Restricted Application added a project: User-DannyS712. · View Herald TranscriptMay 20 2020, 4:42 AM

Restricted Application added a subscriber: Aklapper. · View Herald Transcript

DannyS712 updated the task description. (Show Details)May 20 2020, 4:47 AM

DannyS712 moved this task from Unsorted to Reports on the User-DannyS712 board.

Upstream has two sniffs working with ->

Squiz.WhiteSpace.ObjectOperatorSpacing => already in ruleset.xml
PEAR.WhiteSpace.ObjectOperatorIndent => For multi chain it seems to work, but not for single call over multi lines
- Relevant message: Object operator must be at the start of the line, not the end

Personally, I support the idea of marking -> at the end of the line as an error. However, I don't think this is worth a custom sniff. Such a sniff is not cheap in terms of runtime. The violations are rare. The readability improvement is small. Yes, seeing an -> at the end of the line is a little confusing. But it's not like such code would be unreadable because of this. Let's report this upstream if the two sniffs listed above can't detect this already.

In T253187#6167926, @thiemowmde wrote:

Personally, I support the idea of marking -> at the end of the line as an error. However, I don't think this is worth a custom sniff. Such a sniff is not cheap in terms of runtime. The violations are rare. The readability improvement is small. Yes, seeing an -> at the end of the line is a little confusing. But it's not like such code would be unreadable because of this. Let's report this upstream if the two sniffs listed above can't detect this already.

If it shouldn't be a sniff, would you be willing to review manually fixes?

Hm. I don't know. I did a quick check in my local dev environment and found ~150 exceptions, ~100 of them in core, ~15 in CentralNotice. That's about 0.1% of all ->, but still a lot in my opinion. Do we really win anything by touching these? All it effectively does is moving a newline around.

I would not touch this but leave it to the authors and reviewers.

Upstream https://github.com/squizlabs/PHP_CodeSniffer/issues/2987

Krinkle moved this task from Untriaged to Proposed rule changes on the MediaWiki-Codesniffer board.Jul 16 2020, 10:33 PM

I think this is one of those cases where inconsistency leads to reduced readability and scannability of code. We need to pick one and it seem by and large both our developers, our code bases and the wider PHP community has settled on leading with ->. The exceptions are most likely mistakes. They can be autofixed without further thought. Marking as upstream blocked since this is already a rule for this (PEAR.WhiteSpace.ObjectOperatorIndent) but it does not always work.

If it has an autofix though, we could start with that for now?

Umherirrender unsubscribed.Feb 27 2021, 12:25 PM

Add a sniff for method operator followed by newlineOpen, MediumPublicActions

Description

Event Timeline

Add a sniff for method operator followed by newline
Open, MediumPublic
Actions