With all our work on JS files it's now time to auto-format everything according to #1778828: [policy, no patch] Update JS coding standards.

Postponed on #1778828: [policy, no patch] Update JS coding standards being marked fixed and the standards page being updated.

CommentFileSizeAuthor
#27 core-js-auto-format-2212283-27.patch27.96 KBnod_
#20 core-js-auto-format-2212283-20.patch22.54 KBnod_
#9 core-js-auto-format-2212283-9.patch23.36 KBnod_
#3 core-js-auto-format-2212283-3.patch57.28 KBnod_
core-js-auto-format.patch58.65 KBnod_

Comments

nod_’s picture

nod_
Primary language French
Location Lille
commented
Issue summary: View changes
tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
commented
Status: Needs review » Reviewed & tested by the community

Yay, finally a JS patch that I can review :-)

Looks great.

nod_’s picture

nod_
Primary language French
Location Lille
commented
StatusFileSize
new core-js-auto-format-2212283-3.patch57.28 KB

Reroll.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Reviewed & tested by the community » Needs review 
+++ b/core/misc/dialog.js
@@ -33,7 +33,7 @@
-    function closeDialog(value) {
+    function closeDialog (value) {

Wah?

A space between the function name and its arguments/signature was only meant for anonymous functions?

That is, because for anonymous functions, there is no function name, so we only leave the regular/existing space after "function":

// Anonymous
function ()
// Named
function closeDialog()

Adding a space after all function names would be a total alien to all programmers — I've never seen that style in any common programming language in my life, nor in any JavaScript code in the wild.

nod_’s picture

nod_
Primary language French
Location Lille
commented

I prefer the space because it's easier to pick up and read the function names when you're looking though a file. And to me function declarations are in the parts of code where we should be using proper typographic rules, just like in comments and variable declaration.

"would be a total alien to all programmers" Hyperbole much :p? I'm sure we all read books.

An I hate reading jQuery source code because they switched the spacing of parenthesis.

yesct’s picture

yesct commented
Issue summary: View changes
Status: Needs review » Postponed

I think we need to get #1778828: [policy, no patch] Update JS coding standards marked fixed before continuing here. (OR we auto-format on the old standards and then do it again after agreement on new standards)

the suggestion to always have a space before the ( in a function declaration is illistruated in comment #7 in #1778828: [policy, no patch] Update JS coding standards.

droplet’s picture

droplet commented

NO Space ++.

Hoping it will not be another #2207647: [policy, no patch] Use layout- prefixes instead of l- in CSS Coding Standards.

nod_’s picture

nod_
Primary language French
Location Lille
commented

Fine. I don't really care about that space. The rest of the patch is more important to me.

nod_’s picture

nod_
Primary language French
Location Lille
commented
Status: Postponed » Needs review
StatusFileSize
new core-js-auto-format-2212283-9.patch23.36 KB

Things got resolved in the other issue (as far as what our JS syntax should be) so here is the patch.

For those wondering, function declaration is without space.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Needs review » Reviewed & tested by the community

Excellent.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 9: core-js-auto-format-2212283-9.patch, failed testing.

yesct’s picture

yesct commented

suspicious fail.
and #2015849-16: Some standards fixes in entity_test.module and EntityApiTest.php, motivated by "DatabaseStorageController can't catch exceptions" just failed with that same test.

Drupal\simpletest\Tests\InstallationProfileModuleTestsTest 27 1 0
Message Group Filename Line Function Status
"SystemListingCompatibleTest test executed." found

on testbot 1848 (but the other issue had a different testbot)

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Needs work » Needs review

9: core-js-auto-format-2212283-9.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 9: core-js-auto-format-2212283-9.patch, failed testing.

yesct’s picture

yesct commented

#2239969-20: Session of (UI) test runner leaks into web tests was reverted.

yesct’s picture

yesct commented
Status: Needs work » Needs review

9: core-js-auto-format-2212283-9.patch queued for re-testing.

nod_’s picture

nod_
Primary language French
Location Lille
commented
Status: Needs review » Reviewed & tested by the community

Green, back to rtbc

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 9: core-js-auto-format-2212283-9.patch, failed testing.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented

I played with the idea of re-rolling this, but this patch is auto-generated, right? @nod_?

(awesome, if that's the case)

nod_’s picture

nod_
Primary language French
Location Lille
commented
Status: Needs work » Needs review
StatusFileSize
new core-js-auto-format-2212283-20.patch22.54 KB

It sure is :D

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Needs review » Reviewed & tested by the community

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 20: core-js-auto-format-2212283-20.patch, failed testing.

nod_’s picture

nod_
Primary language French
Location Lille
commented
Status: Needs work » Needs review

20: core-js-auto-format-2212283-20.patch queued for re-testing.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Needs review » Reviewed & tested by the community
corbacho’s picture

corbacho commented

This patch also fixes this bug #2277671: Remove invisible character BM-M- from dialog.position.js
The patch applies nicely. Thumbs up

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 20: core-js-auto-format-2212283-20.patch, failed testing.

nod_’s picture

nod_
Primary language French
Location Lille
commented
Status: Needs work » Needs review
StatusFileSize
new core-js-auto-format-2212283-27.patch27.96 KB

reroll

nod_’s picture

nod_
Primary language French
Location Lille
commented
Status: Needs review » Reviewed & tested by the community
webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Status: Reviewed & tested by the community » Fixed

Really sorry for the delay, folks. My post-DrupalCon life got a bit hectic.

Miraculously though, this still seemed to apply, so committed and pushed to 8.x. Woohoo! :)

  • webchick committed 9c16305 on 8.x 
    Issue #2212283 by nod_: Auto-format JS files.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.