Skip to content
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

Issue #1451: Fix 1388 regression #1452

Closed
wants to merge 4 commits into from

Conversation

julienw
Copy link
Contributor

@julienw julienw commented Jan 2, 2014

No description provided.

case isTypoTypeof(left, right):
warning("W122", this, left.value);
break;
case !eqnull && state.option.eqeqeq:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this change indentation only? Why?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yep, it changes indentation only, because otherwise "npm test" does not pass here :( (this is the first commit in the PR)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should update JSHint. Starting with 2.3 (I think) we determine the indentation style of the switch statement and go with that. So this shouldn't fail npm test (it doesn't on my computer and on Travis).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mmm I somehow thought that npm test was either using the current jshint source or the jshint that is in node_modules ;) My system's jshint is probably old.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should use node_modules JSHint afaik.

@valueof
Copy link
Member

valueof commented Jan 3, 2014

Your description says (not ready to merge) but the patch (indentation change aside) looks fine. Is there something else you'd like to do?

@julienw
Copy link
Contributor Author

julienw commented Jan 3, 2014

Your description says (not ready to merge) but the patch (indentation change aside) looks fine. Is there something else you'd like to do?

Right, I added this before I had a proper fix. Removed the description, and you can merge if it looks fine to you ! :)

@valueof
Copy link
Member

valueof commented Jan 3, 2014

Right, I added this before I had a proper fix. Removed the description, and you can merge if it looks fine to you ! :)

Okay, cool. Will land today.

@valueof
Copy link
Member

valueof commented Jan 3, 2014

Landed both on master and 2.x, thank you!

@valueof valueof closed this Jan 3, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants