Skip to content
This repository was archived by the owner on Jan 19, 2019. It is now read-only.

Fix: add instanceof to ast-converter #251

Merged
merged 1 commit into from
May 4, 2017

Conversation

despairblue
Copy link
Contributor

@despairblue despairblue commented May 4, 2017

The BinaryExpression created by instanceof does not set the operator of the node, because the keyword is missing in TOKEN_TO_TEXT.

This was discovered by prettier/prettier#1480

fixes: #252

@jsf-clabot
Copy link

jsf-clabot commented May 4, 2017

CLA assistant check
All committers have signed the CLA.

@eslintbot
Copy link

Thanks for the pull request, @despairblue! I took a look to make sure it's ready for merging and found some changes are needed:

  • The commit summary needs to begin with a tag (such as Fix: or Update:). Please check out our guide for how to properly format your commit summary and update it on this pull request.

  • Pull requests with code require an issue to be mentioned at the end of the commit summary, such as (fixes #1234). Please update the commit summary with an issue (file a new issue if one doesn't already exist).

Can you please update the pull request to address these?

(More information can be found in our pull request guide.)

@despairblue despairblue changed the title add instanceof to ast-converter Fix: add instanceof to ast-converter May 4, 2017
@soda0289
Copy link
Member

soda0289 commented May 4, 2017

Thanks for the PR.

Can you add the prefix Fix: or New: to your commit.

I think @JamesHenry was working on fixing this is as well. I'll request a review from him just to make sure there is no issue with this change.

@eslintbot
Copy link

Thanks for the pull request, @despairblue! I took a look to make sure it's ready for merging and found some changes are needed:

  • Pull requests with code require an issue to be mentioned at the end of the commit summary, such as (fixes #1234). Please update the commit summary with an issue (file a new issue if one doesn't already exist).

Can you please update the pull request to address these?

(More information can be found in our pull request guide.)

@despairblue despairblue force-pushed the feature/instanceof-keyword branch from ac690ee to e125659 Compare May 4, 2017 18:35
@eslintbot
Copy link

Thanks for the pull request, @despairblue! I took a look to make sure it's ready for merging and found some changes are needed:

  • Pull requests with code require an issue to be mentioned at the end of the commit summary, such as (fixes #1234). Please update the commit summary with an issue (file a new issue if one doesn't already exist).

Can you please update the pull request to address these?

(More information can be found in our pull request guide.)

@despairblue
Copy link
Contributor Author

@soda0289 Thanks, all done.

@soda0289
Copy link
Member

soda0289 commented May 4, 2017

@despairblue
Could you move the test into the folder tests/fixtures/basics as it is not a typescript specific feature.

@despairblue despairblue force-pushed the feature/instanceof-keyword branch from e125659 to 00fabf9 Compare May 4, 2017 18:40
@eslintbot
Copy link

Thanks for the pull request, @despairblue! I took a look to make sure it's ready for merging and found some changes are needed:

  • Pull requests with code require an issue to be mentioned at the end of the commit summary, such as (fixes #1234). Please update the commit summary with an issue (file a new issue if one doesn't already exist).

Can you please update the pull request to address these?

(More information can be found in our pull request guide.)

Copy link
Member

@soda0289 soda0289 left a comment

Choose a reason for hiding this comment

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

LGTM!

@despairblue despairblue force-pushed the feature/instanceof-keyword branch from 00fabf9 to 54b65f6 Compare May 4, 2017 18:42
@eslintbot
Copy link

Thanks for the pull request, @despairblue! I took a look to make sure it's ready for merging and found some changes are needed:

  • Pull requests with code require an issue to be mentioned at the end of the commit summary, such as (fixes #1234). Please update the commit summary with an issue (file a new issue if one doesn't already exist).

Can you please update the pull request to address these?

(More information can be found in our pull request guide.)

@despairblue
Copy link
Contributor Author

@soda0289 thanks, I think @eslintbot is happy now as well :)

@platinumazure
Copy link
Member

@despairblue If you remove the colon between "fixes" and the issue number, eslintbot will be happy. But we can also fix the commit message on merge, so it's up to you if you want to fix it on your end or not. Thanks!

@despairblue despairblue force-pushed the feature/instanceof-keyword branch from 54b65f6 to 92c0c69 Compare May 4, 2017 18:55
@eslintbot
Copy link

LGTM

@despairblue
Copy link
Contributor Author

@platinumazure 💡 thank you, done 😄

@JamesHenry
Copy link
Member

Yeah, that's super weird, I fixed this on a branch but apparently never submitted it as a PR 😄 I have no memory of either making the change nor why I didn't submit it!

@JamesHenry JamesHenry merged commit 00ad71d into eslint:master May 4, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

instanceof is ignored as BinaryExpression.operator
6 participants