Handle '//:lint' and '//:nolint' comments.

Handle '//:lint' and '//:nolint' comments. Currently this only disables 'strictSemicolons' inside 'nolint' zones. The transition from 'nolint' to 'lint' is a little tricky since comments are not tokens, meaning that for proper transition back to 'lint' mode, you need to make sure there is a semicolon (i.e. you have to manually add a token) either before or after the 'lint' to make sure any errors from the 'nolint' side don't leak through. - Add 'ignoreNolint' option. BUG=None TEST=test/feature/Syntax/Error_StrictSemiColonsNoLint.js test/feature/Syntax/StrictSemiColonsNoLint.js

[usrbincc, 2013-03-30 16:45:08]

Note: I'm just cleaning out old issues. New patches will go through github once I get up to speed on it. ---- Some open questions: - Should '//:lint' force on checking? - Should lint and nolint nest? Meaning, should the following be entirely nolint? //:nolint var a //:nolint var b //:lint var c // ? //:lint - Should nolint disable checks inside an imported module? #--cut-- dl_check() { local f=$(basename $1) test -e $f || curl -sO $1 openssl sha1 < $f | grep -q "= $2" && return 0 echo sha1 mismatch! return 1 } git checkout -b issue7801048-nolint dl_check https://codereview.appspot.com/download/issue7801048_1.diff \ d33432ff858dbe5a && git apply issue7801048_1.diff && make test #--cut-- ---- I have two follow-up patches for this, but I'll try submitting them through github. I still need to get comfortable with the new workflow.

[arv, 2013-04-02 17:51:33]

I don't think we want to do this at this layer. Can we either use a third party linter, like jshint or make linting a tool that is run before compiling? I also believe we should fix the parser to keep comments before we do anything based on comments.

[usrbincc, 2013-04-02 18:24:42]

The problem is that there doesn't currently exist a linter that accepts ES6, and certainly not one that's going to keep up with the changes in traceur. Handling comments is a little problematic because you definitely don't want them to be first-class tokens. I'm thinking you'd probably want to attach them as attributes to the first real token that follows. Unless it's a comment at the end of the file, in which case, it would make most sense to attach it as an attribute of the Program, with some metadata to indicate that it's the trailing comment. ---- An alternative, better solution (in my opinion) is to callback into the parser with each comment encountered, and the parser can either look at it or ignore it as needed. As long as we make it a low-cost operation (basically, call the parser with the buffer, and the start and end indexes), it shouldn't be too much of a performance drag. I'm leaving the solution in the previous section just in case it sparks some idea. ---- There are some things that seem easier to do at the scanner level. Things like spaces between operators (and only one space) and line length (including line length of comments, but not those lines containing urls). Theoretically we can do the same thing using source locations, but at first glance, it seems like the solution would be much messier. I'm thinking I'd have to hook into the SourceLocation constructor, and have it update some kind of global state. And that global state needs to know what tokens are associated with those locations. Or I'd have to touch every single function in Parser.js, or add lint self-evaluation code to be generated in ParseTrees.js -- the second sounds a little better, but it's still a little scary.

[usrbincc, 2013-04-05 15:51:31]

I moved the comment-handling into the parser. While this still doesn't save comments, the parser is pretty close to being able to do so (once we hook in 'handleMultiLineComment'). This is a partial implementation of the idea noted in my previous message. #--cut-- dl_check() { local f=$(basename $1) test -e $f || curl -sO $1 openssl sha1 < $f | grep -q "= $2" && return 0 echo sha1 mismatch! return 1 } git checkout af93a2273e581669 # last merge with branch master git checkout -b issue7801048_5001-nolint # I need to figure out how to do this automatically. It's easy for this # to get out of sync. rm test/test-list.js dl_check https://codereview.appspot.com/download/issue7801048_5001.diff \ b7e6878e91d227ba && git apply issue7801048_5001.diff && make test #--cut-- ---- I probably won't get another chance to use this phrase, so: PTAL It's only been what, a week? And I'm already feeling a little nostalgic. https://codereview.appspot.com/7801048/diff/5001/src/options.js File src/options.js (right): https://codereview.appspot.com/7801048/diff/5001/src/options.js#newcode277 src/options.js:277: addBoolOption('ignoreNolint'); I already had this set up for a follow-up patch. It would require a manual merge (I really don't like manual merges) at this point to keep this as a separate commit, so I'm just including it. It's a pretty minor addition, anyway. An odd anecdote: the option was originally named 'ignoreNoLint' ... can you see the bug? I didn't. The problem is that commander.js treats options matching /-no-/ specially, when it should really be matching on /^--no-/. So ./traceur --ignore-no-lint was always false. I started doubting my sanity while chasing down this bug.

[arv, 2013-04-05 16:52:45]

On 2013/04/05 15:51:31, usrbincc wrote: > I moved the comment-handling into the parser. While this still doesn't > save comments, the parser is pretty close to being able to do so (once > we hook in 'handleMultiLineComment'). > I like the parser changes but I'm still not sold that we want to do the //:nolint part. > https://codereview.appspot.com/7801048/diff/5001/src/options.js > File src/options.js (right): > > https://codereview.appspot.com/7801048/diff/5001/src/options.js#newcode277 > src/options.js:277: addBoolOption('ignoreNolint'); > I already had this set up for a follow-up patch. It would require a > manual merge (I really don't like manual merges) at this point to keep > this as a separate commit, so I'm just including it. It's a pretty minor > addition, anyway. > > An odd anecdote: the option was originally named 'ignoreNoLint' ... can > you see the bug? I didn't. > > The problem is that commander.js treats options matching /-no-/ > specially, when it should really be matching on /^--no-/. So > > ./traceur --ignore-no-lint > > was always false. I started doubting my sanity while chasing down this > bug. Wow, another reason we should move away from commmander.js

[usrbincc, 2013-04-05 19:04:35]

> I like the parser changes but I'm still not sold that we want to do > the //:nolint part. Note: I'm assuming here that you're referring to //:nolint as part of traceur, and not the concept of nolint itself. I agree that it's not the cleanest separation of concerns, but there are practical benefits. Piggybacking on the main build means that your compile times remain about the same, because you parse once, not twice. ---- My main motivation for this patch is to be able to turn on strict semicolons checking again in the traceur build. I already have a follow-up patch for 'expand-js-template.js' that inserts nolint and lint around all included files matching a given pattern (/^node_modules/ in our specific case). Okay, I promise, that's the last follow-up patch in this series. I'd like to achieve this goal without impacting build time, especially after so much effort was put into optimizing it. (I'm not overly attached to any particular implementation that meets that criteria.) ---- In general, I'd like to automate as much error-checking as possible, Enabling '--free-variable-checker' would be another advance in this area. and to have the process be as fast as possible. Let the coders be coders, the compilers be compilers, and let the compilers wait on the coders, rather than vice versa. Viva humanity! Down with the machines! So to speak. A command prompt is still better company than many people. ---- > Wow, another reason we should move away from commmander.js I wouldn't be so concerned with this if the pull request queue over at https://github.com/visionmedia/commander.js/pulls wasn't so long. I started putting together a few bugfixes, but kind of got discouraged after seeing the lack of response to the other pulls. I'll probably still give it a try, it's just not a top priority at present. I think part of the issue is that the maintainer is an extremely prolific coder, and of all the projects he's handling, this one doesn't seem to be very high up the list.

[arv, 2013-04-05 19:16:42]

LGTM I'll commit this for you since I have the tools ready

[arv, 2013-04-05 19:29:26]

Committed as f8914ea2fe3287840c6770a900bfbe5ffea7d9dd

[usrbincc, 2013-04-07 01:55:50]

Thanks for the commit. I promise, you'll love traceur's regained semicolon-checking powers so much that you'll forget about the code that makes it possible. I apologize in advance for the extra contortion coming in my next pull request. Here is where I metaphorically lock the doors and turn the lights out before leaving the codereview building for the last time. I'm glad I got the chance to get the codereview experience -- I can see the strengths and weaknesses of github that much more clearly, having something to compare it against. A github pull request corresponds roughly to a single patch set of a codereview issue. I think if github made it possible to deal with pull requests the way codereview deals with patch sets, that would be the ultimate combination. And seriously, github, don't make pieces of the pull request suddenly morph and disappear just because someone did a 'git push -f'! [end rant] I feel like codereview seems to foster a more careful approach. I know that constant iteration is still the side to bet on, but I can't help thinking that this needs to be tempered with periods of more thoughtfulness. I'm sure I'll feel nostalgic about github when the time comes, but we'll cross that bridge when we get to it (when the more frenetic successor to github takes over).