1. Atlassian
  2. Project: Atlassian
  3. aui
  4. Pull requests

Pull requests

#528 Merged at 284d4e6
Repository
Branch
issue/AUI-1933-jshint-2
Repository
Branch
master

Implement JSHint.

Author
  1. Trey Shugart
Reviewers
Description
  • Built upon the issue/AUI-1933-jshint branch that Jonathon Creenaune started.
  • Implement JSHint standards for the Gruntfile.js and src/js/atlassian.js files.
  • Did not implement checking for other files due to the amount of errors. This will be done progressively.
  • Refactor src/js/atlassian.js to not be offensive to the JSHint gods and to 'use strict';.
  • Issues 2 related issues
  • Dependencies Checking for dependencies...
  • Dependents Checking for dependents...

Comments (10)

    1. Trey Shugart author

      Sorry, I tried keeping it down as much as possible, but totally valid point. I'm not aware of an easy way to do that besides moving completely off Bitbucket, which is a viable option given we can still keep everything open to the public.

      -- Trey

  1. Adam Ahmed

    Because of the above bug, I'm commenting here about the JSHint options you chose. I understand this is an opinion issue and am happy to be ignored if you guys have different style preferences.

    I like eqnull for checking against undefined and null at the same time. devel should probably not be enabled globally, but using a directive in the logging methods that need it. Probably newcap as well? I'd imagine most places don't want to allow that. quotmark - I prefer '<html quote="single">single quoted</html>' but "I can't/don't use English (contractions) with it." At least in Stash, it's a nuisance to enforce, but given AUI has like 3 English strings in the whole repo maybe it makes more sense for you guys to enforce. :) forin should be enforced globally, I reckon.

    globals: You marked them all as true. You should list them as false if you want JSHint to know about them, but don't want them to be overwritten by your own code. Probably all should be false except AJS?

    1. Trey Shugart author

      eqnull

      Mostly agree. There’s something about allowing a double equal just for null that doesn’t sit right, but I’m happy to admit that’s personal preference more than anything else. When we come across the need for this we’ll enable it.

      devel

      Agree. This was the “short path” to getting JSHint in AUI ;)

      newcap

      Same as devel. Just need to search out all the Raphael usage and put them there.

      quotmark

      Single, single, single. I’ll have arthritis in my left pinky before the year’s out if double quotes are enforced. In all seriousness, not-having to hold the shift-key every time you type a string makes a difference plus it just adds noise.

      forin

      I was on the fence about that.

      globals

      That’s a good point. I had only ever read the options page on the jshint docs. Guilty. They don’t mention it there (probably should) but they mention in on the docs main page. My bad. Also, console will have to be true since AJS creates it if it doesn’t exist unless jshint will account for that.

      --
      Trey

  2. Adam Ahmed

    Tangent: Possible reason why you guys get more NPM failures than we do. Our dependency versions are either all "~x.x.x" or "x.x.x" - AKA "any acceptable patch version of this" or "this exact version." Once it's been installed, it doesn't have to check the server again. I get output like:

    E:\Code\caviar\etc\lib\qunit-runner>npm install
    
    E:\Code\caviar\etc\lib\qunit-runner>
    

    I think the way you have it isn't documented as supported by NPM (https://npmjs.org/doc/misc/semver.html). So maybe it'll always check the server, which is loose and gives you what you wanted. So you are hitting the server more often than me (maybe). I could definitely be wrong, haven't tested it at all.

    1. Trey Shugart author

      Could be. I’ve seen 2-digit versions used before, but can’t recount where; doesn’t mean it’s correct and the semver spec doesn’t make mention of it. I’ll change these anyways since I’d rather have it conform to the documented standard.

      --
      Trey

    2. Trey Shugart author

      Actually, I just reread that page and it has examples of the shorthand version:

      But the funny thing is, doing require('semver').valid('2.0') returns null, meaning it's invalid. In short, you're right, just a bit odd. Will change to using ~ + the trailing minor version in the meantime. We may just decide to specify the explicit minor version. Dunno.