Use JSHint?

[arv]

JSHint now supports lots of ES6 features. We should investigate to use that instead of //:lint and //:nolint

@usrbincc

[rwaldron]

+1

[rwaldron]

Erik, @guyzmo has been leading the ES6 support implementation for @jshint, so if there are any issues or as more support is needed, he will be the best person to help :)

[valueof]

There's a milestone for the version 2.0: https://github.com/jshint/jshint/issues?milestone=11&state=open Once its done I'll publish it to the NPM. But for now one install straight from the Git repo.

[usrbincc]

My goal with the traceur 'nolint' was to get some minimal checking with little to no increase in compilation time. Those seconds add up when you're iterating.

For me, that minimum would be semicolon checking. Nice to have would be line length (someday, when all the current long lines in the source are fixed). and checking whitespace (or lack thereof) surrounding the proper tokens,. braces, etc. Even nicer to have would be typechecking and variable decl checking (someday, when we add a hook to pre-declare things for FreeVariableChecker).

---

A full-on lint tool would be nice, but if it slows down iteration speed, it's not something I'd want to run on every build, though I'd certainly run it before pull requests (assuming it's been configured to the proper level of pickiness or lack thereof). You may not even want the super picky checks on every build when you're just hacking out code.

On my computer, traceur compilation currently takes 3-4 seconds. It used to take 5-6, which got annoying fast when trying to drill down to the proper places when printf-debugging. Even now, I dream of 1-2 seconds, or impossibly, 1/4 second compiles.

Yes, I don't have to lint all the time, but it's nice having those minimal checks on all the time, because you never forget to do them. When you make a lot of small commits, it makes for less extraneous commit noise not to have to go back and fix semicolon errors and whatnot.

---

So basically it's the same thing I said when we were discussing it on the codereview url for the original nolint patch (f8914ea2fe32), just longer.

---

I wouldn't mind switching over to using jshint-style nolint directives for compatibility. It's too bad they didn't make a fixed prologue to streamline parsing (instead of something that needs a regex even to do the initial go, no-go decision). I really wish they could have picked a more unique tag string, too.

/* jshint doesn't like this comment. */

That aside, it's more of a philosophical thing. The initial check code can still be reaonably efficient, if a little more complex. And for the traceur codebase, we can just use passive voice when talking about jshint.

/* this comment is liked by jshint. */

---

The Rick Waldron! First sighting (by me, at least) of a non-traceur TC39 member I've seen on a traceur topic! (Apologies to anyone I didn't recognize) Without those meeting notes, TC39 would be a complete black box to me. I can't read the spec for more than about 10 minutes. I eventually should just buckle down and tackle it, but the meeting notes are still invaluable for the out-of-band information they provide.

[rwaldron]

@usrbincc :blush:

[guyzmo]

hello,

I think that would be indeed a nice feature to implement, and it would'nt be that hard. I just think we'd need to discuss what would be the best and most elegant syntax to add the nolint directive feature.

Otherwise, jshint does not use regex to parse the directives, but a Pratt algorithm. That means, for the parser to work the most efficiently, it has to meet a token that will change the state of what will be parsed next. So to have a nolint directive after the line not to be checked, we have to either do lookaheads for each statement, or stack all warnings for each line and validate them at each line ending (which would take a few more checks and more memory space).

About making JSHint working faster and only checking a minimal subset of language style features, that could be done, but the best way would be to create a subset of JSHint that does only that minimal checking (which would mean more work to create and maintain). Otherwise, we may investigate if it would be possible, and how much work it would be, to create an option that excludes all expensive checkings and do only the necessary non-expensive minimal checkings that are necessary.

Anyway, for those two options, we can open tickets on jshint's github, and move the discussion over there.

[guyzmo]

btw, I think the :nolint feature can be related to issue https://github.com/jshint/jshint/issues/870

[usrbincc]

@rwldrn I dream of the day when TC39 collectively puts down their word processors and comes over to hack on traceur. Or more realistically, turn the spec into a literate program capable of being used as a compliance suite. Okay, maybe not so realistic.

Maybe I don't really know what I dream of.

It would be very cool to have some TC39 members give some feedback every once in awhile on what features they'd be most interested in test-driving in a live implementation. One of the goals of traceur is "informing the standards process" after all. What better information is there than the feedback of actually using an ES6 feature in a real app?

While I came to traceur with the simple goal of being able to use generators in all browsers, I've started seeing the importance of the other goal. Especially after realizing how much the spec is still in flux.

The IRS has made me see the light. Just about anything is divine ambrosia compared with the tax code. I resolve to devote some time each day (week? month? year? Okay, maybe week) to working through the spec.

I should probably reread the meeting notes after I've done that. Some of it might make more sense.

---

@guyzmo Note that in-traceur checks have an unfair advantage in any speed comparison, because the parsing cost is already paid for in the compile. The extra time for running semicolon checks is not measurable. The only thing that could come close to competing against that would be something compiled.

I see the in-traceur checking and full lint checking as complementary, not substitutes. What I would do if jshint could parse traceur, is run (a suitably configured) jshint as part of my pre-commit hook. I don't commit as often as I compile, so that would be a reasonable trade-off.

And in a pre-commit hook, it'd be easy to only check the changed files, making it even faster. A subset lint might even be fast enough to run on every compile, in the average case.

I might need to add some special rules to include and exclude certain files, but those rules wouldn't have to be updated very often.

In the case of the traceur codebase, that would be enough not to need nolint -- at least to use jshint. Traceur itself would still need it because We can't exclude files we need to compile into bin/traceur.js, or include files that we don't want to compile. I mean, we could, but the implementation would probably be more complicated than nolint, with less flexibility.

The nolint in traceur always comes before the code it affects. It basically sets a parser attribute that gets applied to the parse once the next token has been consumed. Comments are not tokens (and probably shouldn't be anyway, for a compiler) in traceur, so that's the least invasive way I could think of to do it.

I don't use endline comments very often, so I didn't even consider the case of using nolint to affect linting of the same line only. I can see the use of that, though I think the better solution is to just have a special comment that only affects the next line. Just as convenient, but much simpler to code. If you have multiple lines, then you can just use normal nolint-lint markers.

An even better solution would be to change the coding standard so that people don't need to nolint things often enough to need such convenient shortcuts. Or just fix the code to not need the nolint. But that's a different issue.

[arv]

@usrbincc Off topic. As you know, both me, Rick and Alex are part of TC39 and Traceur and other early implementers are influencing TC39.

[usrbincc]

@arv

Off topic.

Should I take this to traceur-compiler-discuss? I've wondered why github doesn't have project discussion pages. It's a prominent (possibly intentional) hole in github's services.

Traceur and other early implementers are influencing TC39.

Maybe it's just that it's hard to see the effects of this influence. At least from outside TC39, it's very hard to tell whether traceur or any other implementation has even indirectly caused members to drop, keep, or modify a feature.

Maybe the influence is more subtle than that, but if so it's even less visible. It's okay that it's not visible. If working on whatever I happen to be interested in is what would best inform the standards process, then I'm fine doing just that. But if there are other things that would be more useful, then I'd be interested in knowing.

[rwaldron]

@usrbincc I can honestly say that I use Traceur at least once a day—every day. It's easily the best ES6 illustration tool that I have :)

[usrbincc]

@rwldrn

Even though optimistically, 1% of the code in there is mine, it's still nice to know that that 1% is getting exercised somewhere out there.

By the way ... have you ever tried up-up down-down left-right left-right a-b (credit arv) during a presentation?

---

@arv

Back on topic. The things missing from jshint as far as I can tell, are classes and modules. Once those land, I think it might be good enough to run on the traceur codebase.

I misinterpreted jshint's option setting comments as being local setting changes. My tests seem to show that the options are set globally once, during an initial source scan, after which the actual checks are run. Or at least something that has the same effect.

cat > asi1.js <<"END"
// jshint asi: false
var a
// jshint asi: true
var b
END
# no errors
jshint asi1.js

cat > asi2.js <<"END"
// jshint asi: true
var a
// jshint asi: false
var b
END
# errors
jshint asi2.js

This is not too big of a deal. Lack of nolint can be worked around.

[valueof]

@usrbincc JSHint's control comments are function scoped.

var a // <-- warning
function test() {
  /* jshint asi:true */
  var b // <-- no warning
}

[rwaldron]

@usrbincc https://github.com/jshint/jshint/issues/1024 and I'll file for modules as soon as the syntax is finalized :)

[valueof]

There's a ticket for modules already. Yehuda keeps bugging me about that.

Anton

On Wednesday, April 17, 2013, Rick Waldron wrote:

@usrbincc https://github.com/usrbincc jshint/jshint#1024https://github.com/jshint/jshint/issues/1024and I'll file for modules as soon as the syntax is finalized :)

— Reply to this email directly or view it on GitHubhttps://github.com/google/traceur-compiler/issues/253#issuecomment-16533567 .

[rwaldron]

@antonkovalyov haha, nice... I would've searched first but thanks for saving me the effort :D

[nschonni]

Now that #266 landed, I was thinking of adding JSHint as another of the Travis build steps run on each commmit. Any thoughts on the options? node is needed for all the require statements Should any other folders be exluded besides the third_party in the .jshintignore file?

@antonkovalyov I'm getting the following error if I try to run it on the src directory:

C:\Users\Nicholas\AppData\Roaming\npm\node_modules\jshint\src\stable\jshint.js:3580                         
                                throw err;                                                                  
                                      ^                                                                     
TypeError: Cannot read property 'value' of undefined                                                        
    at C:\Users\Nicholas\AppData\Roaming\npm\node_modules\jshint\src\stable\jshint.js:1173:23               
    at Object.x.led (C:\Users\Nicholas\AppData\Roaming\npm\node_modules\jshint\src\stable\jshint.js:1114:12)
    at expression (C:\Users\Nicholas\AppData\Roaming\npm\node_modules\jshint\src\stable\jshint.js:839:31)   
    at Object.x.nud (C:\Users\Nicholas\AppData\Roaming\npm\node_modules\jshint\src\stable\jshint.js:2499:6) 
    at expression (C:\Users\Nicholas\AppData\Roaming\npm\node_modules\jshint\src\stable\jshint.js:801:30)   
    at Object.nud (C:\Users\Nicholas\AppData\Roaming\npm\node_modules\jshint\src\stable\jshint.js:2136:16)  
    at expression (C:\Users\Nicholas\AppData\Roaming\npm\node_modules\jshint\src\stable\jshint.js:801:30)   
    at infix.exps (C:\Users\Nicholas\AppData\Roaming\npm\node_modules\jshint\src\stable\jshint.js:2074:19)  
    at Object.x.led (C:\Users\Nicholas\AppData\Roaming\npm\node_modules\jshint\src\stable\jshint.js:1114:12)
    at expression (C:\Users\Nicholas\AppData\Roaming\npm\node_modules\jshint\src\stable\jshint.js:839:31)   

[usrbincc]

@antonkovalyov Function scoping definitely makes it more useful than file scoping. As you can see, I didn't go through a thorough test. Humans don't make very good fuzzers.

In traceur's case, it's possible to wrap the affected pieces of code in functions, but this isn't always possible (or convenient) in general. Say we're concatenating pieces of third-party code that declare variables in the global scope, or within the scope of a single function.

You can still wrap it in an IIFE, and unwrap the "exports" from the return value, but things rapidly get complex if the wrapped fragments expect to be able to interact together in the same scope.

Note that this is just theoretical. I don't really need this, personally.

---

@rwldrn

I'll file for modules as soon as the syntax is finalized

Now that worries me, because one of my near-future goals is to get a rough harmony:modules implementation up. I read through the modules section of es6/2013-03/mar-12.md for what feels like the first time. Must have skimmed it before, but it didn't register.

It looks good, except that you've (general "you") created a virtual file system. That means that all the same file system security issues have to be addressed -- permissions and so on.

The Loader stuff on the wiki is supposedly out-of-date, but there really isn't much detail on it in the meeting notes. I probably need to read more material before I can definitively say it's missing though.

I'll probably have a stronger opinion of this once I get a rough implementation running.

---

@nschonni

Any thoughts on the options?

http://google-styleguide.googlecode.com/svn/trunk/javascriptguide.xml is probably a good place to start.

Mostly semicolons and whitespace formatting and long lines. Can jshint check naming conventions?

Anything more picky will probably get annoying fast, so even if we tighten later, it's probably better to start off lenient. My personal opinion only, though.