[RFC] Unify our JS codestyle with the MW codestyle
Closed, ResolvedPublic

Description

Jonas updated the task description. (Show Details)
Jonas raised the priority of this task from to Needs Triage.
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptAug 29 2015, 8:00 PM

Change 233934 had a related patch set uploaded (by Jonas Kress (WMDE)):
jscs: require space after all keywords, align with wikimedia preset

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

Lydia_Pintscher triaged this task as Normal priority.Sep 1 2015, 12:06 PM
Lydia_Pintscher set Security to None.

I'm for aligning with wikimedia core.

thiemowmde lowered the priority of this task from Normal to Lowest.Sep 1 2015, 3:01 PM

Even if I agree with some details in https://gerrit.wikimedia.org/r/#/c/233934/ (and not with others), I do not want to comment on these details but instead I have to ask:

  • Why was this added to the previous sprint?
  • Why is this in the current sprint?
  • Why do we want to change this? What are the benefits? What is the price? Is it worth it? Who decides if it is worth it?
  • Why do we need to change a code style that was discussed and agreed on in the team before? There is even a wiki page: https://www.mediawiki.org/wiki/Wikibase/Coding_conventions#JavaScript_style_policies. What made the previous consensus invalid?
thiemowmde added a subscriber: adrianheine.EditedSep 4 2015, 10:31 AM

Note that the code style we are messing around with here was intentionally created like that in November 2013. The team changed a lot since then and we may agree that this is reason enough to have this discussion.

This is what I would like to do:

  1. If the relevant developers actually working on JavaScript code (this is Adrian, Jonas and me) agree, I'm all positive to change the rule for control structures to have an enforced space: for (), if (), switch (), while (), also including catch (). https://www.mediawiki.org/wiki/Wikibase/Coding_conventions must be changed then.
  2. A strong no to function () with a space. This contradicts the reasoning behind this "space vs. no space" rule: function declarations and calls never have a space. In PHP public function foo( $param ) does not have a space, calling $this->foo( $param ) does not have a space, so why should function( $param ) have a space? Same in JavaScript, function foo( param ) does not have a space, calling this.foo( param ) does not have a space, so why should foo = function( param ) have a space?

Does https://github.com/jscs-dev/node-jscs/blob/master/presets/wikimedia.json contain a rule for the function() vs. function () ? I not we should add one upstream.

Does https://github.com/jscs-dev/node-jscs/blob/master/presets/wikimedia.json contain a rule for the function() vs. function () ? I not we should add one upstream.

"requireSpaceAfterKeywords": true implies "function". Wikibase's .jscsrc overwrites it, that's what https://gerrit.wikimedia.org/r/233934 is all about.

Now I get it, I was confused by the setting it to true vs an array of words.

In PHP public function foo( $param ) does not have a space, calling $this->foo( $param ) does not have a space, so why should function( $param ) have a space? Same in JavaScript, function foo( param ) does not have a space, calling this.foo( param ) does not have a space, so why should foo = function( param ) have a space?

But in your examples there is always a space after function.

Ricordisamoa added a comment.EditedSep 4 2015, 4:37 PM

A strong no to function () with a space. This contradicts the reasoning behind this "space vs. no space" rule: function declarations and calls never have a space. In PHP public function foo( $param ) does not have a space, calling $this->foo( $param ) does not have a space, so why should function( $param ) have a space? Same in JavaScript, function foo( param ) does not have a space, calling this.foo( param ) does not have a space, so why should foo = function( param ) have a space?

function( param ) could be mistaken for a declaration of a function named "function".
MediaWiki core uses "function" + " " + function name + "(" ecc. and the name of an anonymous function is obviously empty. Also in PHP, because I can find much more occurrences of "function (" than "function(".

Assigning to Adrian as he is the last one who still needs to voice an opinion.

This is just to clarify that I had no intention whatsoever of changing established conventions. I can help you set jscs up for whatever rules you want to follow.

I don't feel strongly about the coding style change. In the long run it probably makes sense to have Wikibase PHP and Wikibase JS as well as Wikibase JS and MediaWiki JS follow the same coding style. I'm not looking forward to the merge conflicts this will give us, though.

long run win > short term pain? :)

My suggestion: Let's stick with https://www.mediawiki.org/wiki/Wikibase/Coding_conventions#JavaScript_style_policies. Thoughts were put into this by the JS developers in our team back then. I'm not against changing it at some point, but we should carefully consider this and sit together.
What I find a good idea definitely is the suggestion by @Ricordisamoa to set up proper jscs rules for whatever conventions we end up using.

My suggestion: Let's stick with https://www.mediawiki.org/wiki/Wikibase/Coding_conventions#JavaScript_style_policies. Thoughts were put into this by the JS developers in our team back then. I'm not against changing it at some point, but we should carefully consider this and sit together.

This is what this ticket is about. Let's decide. Yes back then there were reasons but that's not stopping us from reevaluating.

Tobi_WMDE_SW added a comment.EditedSep 15 2015, 11:49 AM

AFAIK the ticket was created based on a patch that was uploaded before the topic was discussed and by someone who was not aware of our Wikibase JS coding conventions. I get from the conversation on this ticket that for the JS developers there is actually no problem with sticking to that convention so I suggested to not waste more time on this topic for now.

committed for Wikidata-Sprint-2015-09-29: @thiemowmde and @Jonas and @JanZerebecki sit together and decide on this.

Tobi_WMDE_SW removed adrianheine as the assignee of this task.Sep 29 2015, 1:57 PM

Change 242615 had a related patch set uploaded (by Thiemo Mättig (WMDE)):
Enable JSCS rule requireSpaceAfterKeywords

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

Change 242615 merged by jenkins-bot:
Enable JSCS rule requireSpaceAfterKeywords in "google" style

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

The only thing we could not agree on is requireSpaceAfterKeywords for function.

Change 242927 had a related patch set uploaded (by JanZerebecki):
JSCS: requre space also after case, void, with, typeof

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

Change 243083 had a related patch set uploaded (by Ricordisamoa):
build: Enable jscs jsDoc rule 'checkTypes' and make pass

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

Change 243090 had a related patch set uploaded (by Ricordisamoa):
build: Enable jscs jsDoc rule 'checkParamNames' and make pass

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

Change 243097 had a related patch set uploaded (by Ricordisamoa):
build: Enable jscs jsDoc rule 'requireReturnTypes' and make pass

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

Change 243083 merged by jenkins-bot:
build: Enable jscs jsDoc rule 'checkTypes' and make pass

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

Change 243097 merged by jenkins-bot:
build: Enable jscs jsDoc rule 'requireReturnTypes' and make pass

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

Change 243090 merged by jenkins-bot:
build: Enable jscs jsDoc rule 'checkParamNames' and make pass

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

Change 242927 merged by jenkins-bot:
JSCS: require space also after case, void, with, typeof

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

Shall I abandon https://gerrit.wikimedia.org/r/233934 then?

In my opinion, yes. At least for now. We caused quite some trouble and rebases to this point. Would be nice to focus on more productive stuff. Don't get me wrong, I'm thankful for what you did. The fact that an if() in JavaScript and an if () in PHP always looked different always bugged me.

In https://gerrit.wikimedia.org/r/242615 I collected a few more reasons for sticking with function():

  • It's "official" in other JSCS presets, e.g. the "google" preset.
  • JSCS itself uses the "google" preset.
  • Even other big Wikimedia projects like Parsoid and Flow do it like we do.
  • MediaWiki core's PHP uses a mixture of function() and function () with no clear preference and obviously no PHPCS set up to enforce anything. I find this relevant because I consider consistency across languages important.

Change 233934 abandoned by Ricordisamoa:
jscs: require space after "function", align with wikimedia preset

Reason:
T110811#1696595

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

Ricordisamoa updated the task description. (Show Details)
Ricordisamoa removed a subscriber: gerritbot.

For the record, this is what we decided to left out for now: