linemanjs / lineman

Lineman helps you build fat-client JavaScript apps. It produces happiness by building assets, mocking servers, running specs on every file change
MIT License
1.18k stars 82 forks source link

jshint should focus on app/js, not generated/ #220

Closed searls closed 10 years ago

searls commented 10 years ago

When a plugin generates stuff to generated/ (like browserify does), it'll almost always trigger jshint violations that are outside a user's control. Linting generated code seems nebulous.

Silver lining: I am impressed that 100% of the generated coffeescript code I've written with lineman for the last two years has apparently all passed jshint.

jasonkarns commented 10 years ago

intention being to ignore vendor stuff or app coffee as well?​

searls commented 10 years ago

Ignore generated stuff, period. Honestly, the handful of times I've seen it blow up over vendor stuff, there's wasn't any more that I could do. Propose we just cover app/js since it's what the user owns.

On Tue, Feb 25, 2014 at 1:33 PM, Jason Karns notifications@github.com wrote:

intention being to ignore vendor stuff or app coffee as well?​

Reply to this email directly or view it on GitHub: https://github.com/linemanjs/lineman/issues/220#issuecomment-36041381

jasonkarns commented 10 years ago

Well, the meat of my question is: should we be running jshint on compiled coffeescript (et. al.)

It doesn't really make sense to run a linter on compiled JS, but we ought to be crystal clear to the users that anything pre-compiled isn't linted.

searls commented 10 years ago

Answer: we should run jshint on JavaScript under app/js. That was always my intention and I apparently got carried away in the original settings.

On Tue, Feb 25, 2014 at 2:25 PM, Jason Karns notifications@github.com wrote:

Well, the meat of my question is: should we be running jshint on compiled coffeescript (et. al.) It doesn't really make sense to run a linter on compiled JS, but we ought

to be crystal clear to the users that anything pre-compiled isn't linted.

Reply to this email directly or view it on GitHub: https://github.com/linemanjs/lineman/issues/220#issuecomment-36047202

searls commented 10 years ago

Well, sure enough I actually did what I intended to do. JSHint is configured to look at files.js.app

$ lineman config files.js.app
app/js/**/*.js

But @davemo's Browserify plugin changes this path to match:

$ lineman config files.js.app
generated/js/browserifyBundle.js

I think we should figure out how to make browserify work without overriding the one file pattern we have to indicate "JS that lives under app/" -- @davemo?

jayharris commented 10 years ago

There's always CoffeeLint for the coffee files.

searls commented 10 years ago

The more I think about it, the less I can imagine us coming up with a better file pattern for browserify without duplicating all the complexities of what browserify does.

searls commented 10 years ago

Perhaps we should just change lineman-browserify's jshint config to look at the old app/js... that seems more reasonable

davemo commented 10 years ago

I didn't want to just use the standard lineman configuration for where to put the output browserify bundle to because I figured people would want to put either multiple bundles in place and/or use the 1 browserifyBundle.js in conjunction with old code that is output into generated/js/app.js

davemo commented 10 years ago

One of the goals of lineman-browserify was to give people an easy answer to the question of "how do i have multiple bundles configured?"

searls commented 10 years ago

I think that's a separate point entirely Dave and not without merit. Here we're only talking about the issue of "files.js.app" being a generated source file now that's out of the user's hands versus the default which puts it in app/js

searls commented 10 years ago

It may be that this only makes a mess of things with jshint and nothing else, which is what I'm hoping

davemo commented 10 years ago

Tangentially, in the case of browserify if there are problems parsing I'm pretty sure its tree walking and concatenating would barf, but I haven't tested this theory.

searls commented 10 years ago

Closing for now. Will address this in lineman-browserify