Reimplement missing code from jshint.js #2
Conversation
ajakaja
changed the title
There was a problem hiding this comment.
Hey Alex,
This is great work! I think it's almost ready for upstream. Here's how I'd like to move forward:
I like all of your ideas for future improvements, and I admire your restraint in not implementing them right now. That said, we generally prefer to document plans for future work using the project issue tracker. I'd like to wait a bit before directing you to contribute to the upstream codebase directly, so in the mean time, I've filed a couple issues against this project: gh-3 and gh-4 (I've also reported those in the upstream repository). If you'd like to take them on, I would gladly accept the patches here and re-land them upstream. In any event, would you mind removing the "TODO"s from this patch?
Separately, the patch currently fails the project's linting checks as reported by the command npm test (or npm run pretest). Would you mind fixing those?
With those things out of the way, here's how I'd like to merge your work (I don't expect you to do this part, but I still want to make sure it's okay with you). We've set out to re-implement existing functionality, and that makes me sensitive to the potential for regression. As you know, that's a risk I'm willing to take, but we should do our best to mitigate it. You've done your part by getting the tests to pass and documenting your process. For my part, I'd like to be particularly precise with how it gets merged. I'm going to land it as three distinct commits:
- typo corrections
- implementation of the missing functionality
- enhancement of the test harness
My hope is that will help demonstrate the equivalency of your implementation without losing any of the other improvements you've contributed. Does that sound okay with you?
src/jshint.js
Outdated
| advance(); | ||
| if (state.tokens.next.value === "{") { | ||
| // manually cheating the test "invalidClasses", which asserts this particular behavior when a class is misdefined. | ||
| // this is completely ridiculous, but I don't want to change any tests right now. |
src/jshint.js
Outdated
| // so I'm just saving it on the state directly. | ||
| // state.inObjectBody is distinct from state.inClassBody because .inClassBody also causes strict mode to be used, which is | ||
| // not correct for object literals. | ||
| if (!state.inClassBody && !state.inObjectBody && !isMethod()) { |
There was a problem hiding this comment.
Your suspicion of state.inClassBody is well-founded. However, I'm willing to accept this because the existing state.inObjectBody sets a clear precedent for this and because the ways it fails are fairly specific. I'm happy to to improve the general pattern shared by both flags myself.
src/jshint.js
Outdated
| className = classNameToken.value; | ||
| } | ||
|
|
||
| //TODO: the constructor's 'name' should be the name of the variable in an assignment expression, ie in 'var x = class Y {}' I believe it's supposed to be 'x'. |
There was a problem hiding this comment.
This is a lot like a question someone asked on the blog post you found!
What would the name be of the following:
var myVar = function myFunc(){};
In your example, the name would be "Y". V8 (and SpiderMonkey) get this right:
$ node -p 'var x = class Y {}; x.name'
Y
src/jshint.js
Outdated
| advance(); | ||
| break; | ||
| case "constructor": | ||
| if (isStatic || inGenerator) { |
There was a problem hiding this comment.
Actually, static methods may be named "constructor", so (confusing though it may be) we'll need to allow that.
|
That all sounds good. I will remove TODOs and -- whoops, I was using 'test-all' to run the tests and forgot to run 'test' again to check for the linter. |
Hey,
Got everything working, more or less, though I was gaining understanding of the API as I went so there are definitely some misunderstandings still in there. But all the tests passed!
(Well -- the test-262 tests did not run; I think they're broken in this branch. Haven't looked into it yet.)