Permalink
Browse files

Update grunt-contrib-jshint to 0.7.1 and squash jshint tasks.

  • Loading branch information...
1 parent 1f724ad commit 61d16ec939c11f63419cae5bc2aa990134f6c0ff @scottgonzalez scottgonzalez committed Nov 1, 2013
Showing with 9 additions and 24 deletions.
  1. +8 −23 Gruntfile.js
  2. +1 −1 package.json
View
@@ -176,30 +176,15 @@ grunt.initConfig({
})
},
jshint: {
- ui: {
- options: {
- jshintrc: "ui/.jshintrc"
- },
- files: {
- src: "ui/*.js"
- }
- },
- grunt: {
- options: {
- jshintrc: ".jshintrc"
- },
- files: {
- src: [ "Gruntfile.js", "build/**/*.js" ]
- }
+ options: {
@mgol

mgol Nov 10, 2013

Member

@scottgonzalez Is it correct? jshint is a multi-task, it should always have a target (there may be only one of them but it should exist).

@mgol

mgol Nov 11, 2013

Member

Sorry, I commented on the wrong line. All grunt mutli-tasks support default options by default after all.

The real issue is this configuration looks like no target was needed at all which is not correct because jshint is a multi-task. What happens here is src is the only target and it just happens that jshint accepts a configuration in which an array of files is dumped directly onto the target. The src name here is irrelevant but being so similar to the way you provide files info to Grunt tasks, it puzzles developers making them think it's different to what it really is (I know it puzzled me).

I'd prefer to have it like:

all: {
    src: [
        "ui/*.js",
         "Gruntfile.js",
         "build/**/*.js",
         "tests/unit/**/*.js"
    ]
}

to support best Grunt practices but if you insist on a shorter form, the following would still IMO be better to what's here currently:

all: [
     "ui/*.js",
     "Gruntfile.js",
     "build/**/*.js",
     "tests/unit/**/*.js"
]
@scottgonzalez

scottgonzalez Nov 11, 2013

Owner

I have a hard time believing that this is violating some best practice, but I'll defer to @shama, @tkellen, or @cowboy.

@shama

shama Nov 11, 2013

src is a valid target name although I recommend against it as I agree, it causes confusion. Also there have been a number of requests for no target multitask config support. It's not likely to land in Grunt but just in case, avoiding those as target names is tiny bit more future proof.

@scottgonzalez

scottgonzalez Nov 11, 2013

Owner

Ok, so we'll just rename src to all.

@tkellen

tkellen Nov 11, 2013

Member

Cool. That's the form we're using for pretty much all of the contrib tasks.

+ jshintrc: true
},
- tests: {
- options: {
- jshintrc: "tests/.jshintrc"
- },
- files: {
- src: "tests/unit/**/*.js"
- }
- }
+ src: [
+ "ui/*.js",
+ "Gruntfile.js",
+ "build/**/*.js",
+ "tests/unit/**/*.js"
+ ]
},
csslint: {
base_theme: {
View
@@ -57,7 +57,7 @@
"dependencies": {},
"devDependencies": {
"grunt": "0.4.1",
- "grunt-contrib-jshint": "0.6.3",
+ "grunt-contrib-jshint": "0.7.1",
"grunt-contrib-uglify": "0.1.1",
"grunt-contrib-concat": "0.1.3",
"grunt-contrib-qunit": "0.2.0",

0 comments on commit 61d16ec

Please sign in to comment.