Follow-up of #1664940: [Policy, patch] Decide on JSHint configuration and part of #1415788: 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

CommentFileSizeAuthor
#22 core-jshint-drupal-1684800-22.patch361 bytesnod_
#19 core-jshint-drupal-1684800-19.patch351 bytesnod_
#17 core-jshint-drupal-1684800-17.patch280 bytesnod_
#14 jshint_drupal_js-1684800-14.patch352 bytesfoopang
#5 core-js-jshint-drupal.patch393 bytesnod_

Comments

nod_’s picture

nod_
Primary language French
Location Lille
commented
Title: drupal.js » JSHint drupal.js
nod_’s picture

nod_
Primary language French
Location Lille
commented 
core/misc/drupal.js: line 172, col 17, Expected a 'break' statement before 'default'.
droplet’s picture

droplet commented 
      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

nod_
Primary language French
Location Lille
commented

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

nod_’s picture

nod_
Primary language French
Location Lille
commented
Status: Active » Needs review
StatusFileSize
new core-js-jshint-drupal.patch393 bytes
droplet’s picture

droplet commented
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

droplet commented
Status: Needs work » Needs review

Seems like a random string test fails.

droplet’s picture

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

droplet commented

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

catch’s picture

catch
he/him
Primary language English
commented
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

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

foopang commented
Status: Needs work » Needs review
StatusFileSize
new jshint_drupal_js-1684800-14.patch352 bytes

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

droplet’s picture

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

Redefinition ??

jelle_s’s picture

jelle_s
Primary language Dutch
Location Antwerp, Belgium
commented

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

nod_
Primary language French
Location Lille
commented
Status: Postponed (maintainer needs more info) » Needs review
StatusFileSize
new core-jshint-drupal-1684800-17.patch280 bytes

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

droplet commented

ahh. never know it

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

nod_’s picture

nod_
Primary language French
Location Lille
commented
StatusFileSize
new core-jshint-drupal-1684800-19.patch351 bytes

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

nod_’s picture

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

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
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

nod_
Primary language French
Location Lille
commented
StatusFileSize
new core-jshint-drupal-1684800-22.patch361 bytes

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

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

Let's pretend that never happened :)

nod_’s picture

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

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

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

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
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.