Page MenuHomePhabricator

Standardize return type hint spacing
Closed, ResolvedPublic

Description

Currently both ): and ) : are used in our code for return type hints. We should standardize on one of those and a have a sniff for it.

Please discuss here Please vote.

(If you can't vote due to Phabricator's somewhat clumsy antispam system, please say so in a comment so you can be given permission.)

{V25}

Event Timeline

I quote from https://www.mediawiki.org/wiki/Manual:Coding_conventions/CSS:

We love whitespace.

And from https://www.mediawiki.org/wiki/Manual:Coding_conventions/PHP

MediaWiki favors a heavily-spaced style for optimum readability.

Therefore, it seems like a no-brainer to use function () : type.

Return type hints are used more and more. I would also support more spaces here ") : "

Can we avoid having the discussion in two places, please? There’s already a lot more arguments on the mw.org talk page (also linked in the task description).

I evaluated my arguments from the mw.org talk page and still think the character sequence … ) : … is not automatically better. I still suggest to follow PSR and enforce … ): ….

Can we avoid having the discussion in two places, please? There’s already a lot more arguments on the mw.org talk page (also linked in the task description).

Marking stalled to highlight it’s blocked on a discussion. Desc tweaked

DannyS712 changed the task status from Open to Stalled.Dec 2 2019, 11:55 AM

Can we avoid having the discussion in two places, please? There’s already a lot more arguments on the mw.org talk page (also linked in the task description).

Marking stalled to highlight it’s blocked on a discussion. Desc tweaked

Discussion was archived without any result

Reedy triaged this task as Low priority.Apr 2 2021, 3:22 AM

Can we avoid having the discussion in two places, please? There’s already a lot more arguments on the mw.org talk page (also linked in the task description).

Marking stalled to highlight it’s blocked on a discussion. Desc tweaked

Discussion was archived without any result

Specifically, https://www.mediawiki.org/wiki/Manual_talk:Coding_conventions/PHP/Archive#Declaring_of_return_types was archived by @Krinkle because he decided to collapse all the talk pages onto one.

In the absence of consensus, how about we just go with spaces for now?

Change 679844 had a related patch set uploaded (by DannyS712; author: DannyS712):

[mediawiki/tools/codesniffer@master] Add new FunctionTypehintWhiteSpaceSniff

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

The upstream sniff PSR12.Functions.ReturnTypeDeclaration is for PS12 without spaces between ) and :

But it could be used to give some ideas on how to do it.

It’s probably worth mentioning here that Wikibase already standardized on without-space earlier this year (I4d48fe9d0c), using slevomat/coding-standard.

Can we avoid having the discussion in two places, please? There’s already a lot more arguments on the mw.org talk page (also linked in the task description).

Marking stalled to highlight it’s blocked on a discussion. Desc tweaked

Discussion was archived without any result

Specifically, https://www.mediawiki.org/wiki/Manual_talk:Coding_conventions/PHP/Archive#Declaring_of_return_types was archived by @Krinkle because he decided to collapse all the talk pages onto one.

In the absence of consensus, how about we just go with spaces for now?

+1

Jdforrester-WMF changed the task status from Stalled to Open.Jun 14 2021, 7:48 PM

OK, marking as un-Stalled.

In the absence of consensus, how about we just go with spaces for now?

How about we don't act as if we have consensus if we don't have consensus?

In the absence of consensus, how about we just go with spaces for now?

How about we don't act as if we have consensus if we don't have consensus?

Since the onwiki discussion was never resolved, and its hard to sense consensus from a thread like on phabricator, perhaps we should use a vote (https://phabricator.wikimedia.org/vote/) ? I can create one if there are no objections

I did not explain my point of view enough. Sorry.

This is a bit like the famous "tabs vs. spaces" discussion. In a situation like this where neither option can ever be a clear winner it might indeed be a good idea to just go with one of the two options. Sometimes it's much more important to make a decision, no matter which one it is.

However, this doesn't apply here.

  1. To do this we still need to ask if people want a decision. Mixing tabs and spaces for example creates actual problems. But this here? What happens when we let people decide if they want the space in their code or not?
  2. It's not like the two options are equal and we are free to pick whatever we feel like. There is an industry standard. I believe we should either follow the standard or explain why we think the standard doesn't apply, and what we win by intentionally not following it.

CSS convention does not apply to PHP

And from https://www.mediawiki.org/wiki/Manual:Coding_conventions/PHP

MediaWiki favors a heavily-spaced style for optimum readability.

Therefore, it seems like a no-brainer to use function () : type.

I don't think this deductive submission is valid. If it were, then ( bool ) $foo would have already been allowed on similar premise.

Let's go with the voting, I don't think debating the supposed merits of space vs. no space gets us anywhere.

To do this we still need to ask if people want a decision. Mixing tabs and spaces for example creates actual problems. But this here? What happens when we let people decide if they want the space in their code or not?

We can include a "don't standardize" voting option.

I've created V25 - but as I learned at T284976 votes are only visible to Trusted-Contributors and WMF-NDA so not everyone will be able to participate

I voted, but I don't think a poll can explain why we think it's a good idea to violate an industry standard. You need to explain this, or have a clear majority (e.g. two third) that includes more than the half dozen devs that are watching this here. Otherwise I will continue to respect PSR in the codebases I'm responsible for.

What's the duration of the vote? How will people (beyond those subscribed here know a polling is going on?)

I voted, but I don't think a poll can explain why we think it's a good idea to violate an industry standard. You need to explain this, or have a clear majority (e.g. two third) that includes more than the half dozen devs that are watching this here. Otherwise I will continue to respect PSR in the codebases I'm responsible for.

+1

I second Thiemo's and Ammarpad's comments. I suggest against giving any significance to the poll, as right now, it's not sufficiently announced.

Also, I'm stashing an essay here: https://meta.wikimedia.org/wiki/Polls_are_evil.

  1. To do this we still need to ask if people want a decision. Mixing tabs and spaces for example creates actual problems. But this here? What happens when we let people decide if they want the space in their code or not?

We'll have the inconsistency we had before, where one group picks one style and one group picks another style. Admittedly with PSR-12, this is less likely (since it is broadly adopted), but our standard precedes that. PHP is inconsistent enough as it is already, let's not add more inconsistencies in our code.

  1. It's not like the two options are equal and we are free to pick whatever we feel like. There is an industry standard. I believe we should either follow the standard or explain why we think the standard doesn't apply, and what we win by intentionally not following it.

We haven't bothered to follow the standard before, why should we start now? If we do follow it, we'll be internally inconsistent for the benefit of following an industry standard we don't actually use. (Minus a few shared rules perhaps).

CSS convention does not apply to PHP

It was more to illustrate the point.

MediaWiki favors a heavily-spaced style for optimum readability.

Therefore, it seems like a no-brainer to use function () : type.

I don't think this deductive submission is valid. If it were, then ( bool ) $foo would have already been allowed on similar premise.

That comparison was meant to refer to related language features such as function arguments, function calls and brace placement, which all have spaces in them.

I second Thiemo's and Ammarpad's comments. I suggest against giving any significance to the poll, as right now, it's not sufficiently announced.

We should probably send a message to wikitech-l.

MediaWiki favors a heavily-spaced style for optimum readability.

Therefore, it seems like a no-brainer to use function () : type.

I don't think this deductive submission is valid. If it were, then ( bool ) $foo would have already been allowed on similar premise.

That comparison was meant to refer to related language features such as function arguments, function calls and brace placement, which all have spaces in them.

I believe your submission T220719#5104780 does not seem to agree with what you're saying now. Additionally, even in "function calls" it's not true that they're heavily spaced; foo () is disallowed albeit it's valid syntax in PHP.

Hi, I can't view that V25.

We should probably send a message to wikitech-l.

Done (also to the MediaWiki Stakeholders channel).

Hi, I can't view that

Should be fixed now.

While I personally prefer the spaced-out version, I'm also a big fan of following specifications unless they're significantly hampering the project. So, if the PSR-12 says no space, then I'd go with that.

Also, I'm not seeing anything that looks like a vote, but I'm not sure if I've ever actually voted in Phabricator before, so I might just be missing it. Can someone grant me permissions, just to be sure? Thanks!

I've voted for the "without a space" variant like function ( Aa $a, Bb $b ): Type.

My thinking:

  • PSR standards relating to coding style don't apply to MW. Merging two different design guides generally does not work well to reduce cognitive overhead. As such, this did not play a role for me.
  • I am generally in favour of the spacious coding style that MediaWiki follows.
  • But – my interpretation of our spacious style is that we add spaces to separate logical blocks that we consider "useful" in some way. Our style is not to add a space literally everywhere where the language allows. For example, we don't add spaces on both sides of a comma, or before a semicolon, etc. $a = [ $b , $c ] ;.
  • I think our style calls for Type to be separated by a space from :. However I see no reason to separate ): from each other as I do not see the colon as a useful visual indicator to set apart by itself. When a type is followed after the signature, the presence of that type is what visually stands out as the distinguishing detail. I suppose one could think of it a bit like how => and = are both forms of assignment, just one is slightly different. Similarly ): would be the end of the parameter list for when a return type follows after it.

I'm not seeing anything that looks like a vote […]

Fixed. Try again now :)

Fixed. Try again now :)

That's got it. Thanks!

There doesn't seem to be a way to track the time distribution of votes, but I think there hasn't been much change in the last several days, and with the "no space" option having a pretty conclusive lead (56% vs. 28% for "yes space"), I think we can call it. Updated the code style docs.

There doesn't seem to be a way to track the time distribution of votes, but I think there hasn't been much change in the last several days, and with the "no space" option having a pretty conclusive lead (56% vs. 28% for "yes space"), I think we can call it. Updated the code style docs.

I've closed the poll so that we don't get new votes, and will update my phpcs patch

Change 704370 had a related patch set uploaded (by DannyS712; author: DannyS712):

[mediawiki/tools/codesniffer@master] Remove spaces before return typehints in existing code

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

Change 704370 merged by jenkins-bot:

[mediawiki/tools/codesniffer@master] Remove spaces before return typehints in existing code

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

Change 679844 merged by jenkins-bot:

[mediawiki/tools/codesniffer@master] Add new FunctionTypehintWhiteSpaceSniff

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

Change 704557 had a related patch set uploaded (by Daimona Eaytoy; author: Daimona Eaytoy):

[mediawiki/tools/codesniffer@master] Use built-in PSR12.Functions.ReturnTypeDeclaration for typehint spacing

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

Change 704557 merged by jenkins-bot:

[mediawiki/tools/codesniffer@master] Use built-in PSR12.Functions.ReturnTypeDeclaration for typehint spacing

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

Change 705734 had a related patch set uploaded (by Jforrester; author: Jforrester):

[labs/libraryupgrader/config@master] Bump mediawiki-codesniffer from 36.0.0 to 37.0.0

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

Change 705734 merged by jenkins-bot:

[labs/libraryupgrader/config@master] Bump mediawiki-codesniffer from 36.0.0 to 37.0.0

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