Follow-up of #1664940: [Policy, patch] Decide on JSHint configuration and part of #1415788: [Meta] javascript winter clean-up

Run jshint on the files with the configuration from the parent issue or use jshint.com with the following options:

/*jshint forin:true, noarg:true, eqeqeq:true, undef:true, curly:true, browser:true, expr:true, latedef:true, newcap:true, trailing:true */
/*global Drupal, jQuery */

Fix any warnings or errors the tool finds.
Check manually that the fixes did not break any functionalities
Create patch and upload for the testbot.

Files: drupal.js

Files: 
CommentFileSizeAuthor
#22 core-jshint-drupal-1684800-22.patch361 bytesnod_
PASSED: [[SimpleTest]]: [MySQL] 55,819 pass(es).
[ View ]
#19 core-jshint-drupal-1684800-19.patch351 bytesnod_
PASSED: [[SimpleTest]]: [MySQL] 55,706 pass(es).
[ View ]
#17 core-jshint-drupal-1684800-17.patch280 bytesnod_
PASSED: [[SimpleTest]]: [MySQL] 54,220 pass(es).
[ View ]
#14 jshint_drupal_js-1684800-14.patch352 bytesfoopang
PASSED: [[SimpleTest]]: [MySQL] 53,643 pass(es).
[ View ]
#5 core-js-jshint-drupal.patch393 bytesnod_
PASSED: [[SimpleTest]]: [MySQL] 37,258 pass(es).
[ View ]

Comments

nod_’s picture

Title:drupal.js» JSHint drupal.js
nod_’s picture

core/misc/drupal.js: line 172, col 17, Expected a 'break' statement before 'default'.
droplet’s picture

      switch (key.charAt(0)) {
        // Escaped only.
        case '@':
          args[key] = Drupal.checkPlain(args[key]);
        break;
        // Pass-through.
        case '!':
          break;
        // Escaped and placeholder.
        case '%':
        default:
          args[key] = Drupal.theme('placeholder', args[key]);
          break;
      }

It works fine. It can have a better description or comment out to avoid JSHint warning

nod_’s picture

You just need to remove the case '%': line, this is what's called fall through and that's not needed here.

nod_’s picture

Status:Active» Needs review
StatusFileSize
new393 bytes
PASSED: [[SimpleTest]]: [MySQL] 37,258 pass(es).
[ View ]
droplet’s picture

Status:Needs review» Reviewed & tested by the community

Status:Reviewed & tested by the community» Needs work

The last submitted patch, core-js-jshint-drupal.patch, failed testing.

droplet’s picture

Status:Needs work» Needs review

Seems like a random string test fails.

droplet’s picture

Status:Needs review» Reviewed & tested by the community
droplet’s picture

#5: core-js-jshint-drupal.patch queued for re-testing.

catch’s picture

Status:Reviewed & tested by the community» Fixed

Committed/pushed to 8.x, thanks.

Status:Fixed» Closed (fixed)

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

nod_’s picture

Status:Closed (fixed)» Needs work
core/misc/drupal.js: line 1, col 5, Redefinition of 'Drupal'.
foopang’s picture

Status:Needs work» Needs review
StatusFileSize
new352 bytes
PASSED: [[SimpleTest]]: [MySQL] 53,643 pass(es).
[ View ]

I'm not sure how it should be fixed, but I wanna give it a try. Please review the patch attached.

droplet’s picture

Status:Needs review» Postponed (maintainer needs more info)

Redefinition ??

Jelle_S’s picture

I think this has to do with the configuration (#1664940: [Policy, patch] Decide on JSHint configuration). I'm not sure if you can use a different config for just one file, but it seems that, because we define Drupal as 'predefined', jsHint isn't happy when we "redefine" it.

For more background info see http://jslinterrors.com/redefinition-of-a/
Not sure if the patch in #14 is the correct way to deal with it...

nod_’s picture

Status:Postponed (maintainer needs more info)» Needs review
StatusFileSize
new280 bytes
PASSED: [[SimpleTest]]: [MySQL] 54,220 pass(es).
[ View ]

doesn't seem like there is much we can do about that for now.

Patch makes the error go away as per Jelle_S link's instructions. Not sure that's the best option though.

droplet’s picture

ahh. never know it

export Drupal to window it seems to be a better approach. ( window.Drupal === Drupal // TRUE )

nod_’s picture

StatusFileSize
new351 bytes
PASSED: [[SimpleTest]]: [MySQL] 55,706 pass(es).
[ View ]

New version of jshint (2.0) allows for warnings to be turned on and off, this patch works.

nod_’s picture

Status:Needs review» Reviewed & tested by the community
alexpott’s picture

Status:Reviewed & tested by the community» Needs review

The problem is we have to say we need the latest and greatest jshint... otherwise you get

core/misc/drupal.js: line 3, col 1, Bad option: '+W079'.

And this warning is then suppressed...

So is there anyway to declare / document the dependency on jshint > 2.0.0 in the .jshint file?

nod_’s picture

StatusFileSize
new361 bytes
PASSED: [[SimpleTest]]: [MySQL] 55,819 pass(es).
[ View ]

Since the .jshintrc file is a JSON file we can't comment it. I've looked around and there isn't another way to restrict jshint version.

I updated the doc page saying the latest stable version of JSHint should be used: http://drupal.org/node/1972428 (and updated the config changes from #1995996: Update JSHint configuration as well as the change notice: http://drupal.org/node/1972428).

But all that doesn't matter, droplet had the solution in #18. Updated patch that doesn't need special comments or a specific version of jshint.

Status:Needs review» Needs work

The last submitted patch, core-jshint-drupal-1684800-22.patch, failed testing.

nod_’s picture

Status:Needs work» Needs review

Let's pretend that never happened :)

nod_’s picture

Status:Needs review» Reviewed & tested by the community
alexpott’s picture

#22: core-jshint-drupal-1684800-22.patch queued for re-testing.

alexpott’s picture

Status:Reviewed & tested by the community» Fixed

Committed c785642 and pushed to 8.x. Thanks!

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