New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[[FEAT]] support destructuring in ForIn/Of loops, lint bad ForIn/Of LHS #2341

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
4 participants
@caitp
Copy link
Member

caitp commented Apr 25, 2015

Blocked on #2326 (test failures due to "const" not being block scoped)

Things to consider: maybe treat destructuring ForIn loops as "bad" and report an error? There isn't really any point to destructuring those

@caitp caitp force-pushed the caitp:for-inof-lhs-destructuring branch 2 times, most recently from 2c98590 to a0af488 Apr 25, 2015

@@ -4186,6 +4215,15 @@ var JSHINT = (function() {
error("W104", nextop, "for of");
}

var ok = !(initializer || comma);
if (initializer) {

This comment has been minimized.

@jugglinmike

jugglinmike Apr 25, 2015

Member

How about

-if (initializer) {
+if (initializer && state.option.inESNext()) {

This comment has been minimized.

@caitp

caitp Apr 25, 2015

Member

technically the initializer is legal in es5.1, but I think we should lint it anyways, since it's meaningless in an enumeration context

This comment has been minimized.

@jugglinmike

jugglinmike Apr 25, 2015

Member

Yeah, it wouldn't hurt to be opinionated about this. I was worried this would be a breaking change, but I just checked and JSHint doesn't allow this now (just with a less helpful error, "Expected 'in' and instead saw '='."). The only concern I have is that users cannot ignore errors. I don't know if defining an analogous warning for this problem is really worthwhile, though, because I can't think of any case where someone would intentionally want this.

@jayphelps

This comment has been minimized.

Copy link

jayphelps commented May 8, 2015

This should be unblocked now 😄 via #2326 (comment)

@jayphelps

This comment has been minimized.

Copy link

jayphelps commented May 8, 2015

Huge thanks for doing this btw. I'll finally get to turn jshint back on! 🎉

@caitp caitp force-pushed the caitp:for-inof-lhs-destructuring branch from a0af488 to 4e10532 May 9, 2015

@caitp caitp force-pushed the caitp:for-inof-lhs-destructuring branch from 4e10532 to 0b1df3e May 9, 2015

@caitp

This comment has been minimized.

Copy link
Member

caitp commented May 9, 2015

rebased, and tests pass locally --- so this is probably good to land tonight. I'll check this in after travis is green, and we can revert if needed, but I'm pretty sure we're cool with it

@coveralls

This comment has been minimized.

Copy link

coveralls commented May 9, 2015

Coverage Status

Coverage increased (+0.01%) to 96.47% when pulling 0b1df3e on caitp:for-inof-lhs-destructuring into 162e8f7 on jshint:master.

@caitp caitp closed this in c0edd9f May 9, 2015

jugglinmike added a commit to jugglinmike/jshint that referenced this pull request May 9, 2015

[[FEAT]] support destructuring in ForIn/Of loops, lint bad ForIn/Of LHS
Adds support for destructuring bindings in for loops

Closes jshint#2341
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment