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

Update: add proper scope analysis (fixes #535) #540

Merged
merged 31 commits into from
Nov 13, 2018
Merged

Conversation

mysticatea
Copy link
Member

@mysticatea mysticatea commented Nov 5, 2018

@mysticatea
Copy link
Member Author

I'm a newbie about jest. Please advise me on how to write tests with jest.
Currently, I use snapshot tests with the scope tree after I convert circular references to { $id: X ... } and { $ref: X }.

Copy link
Member

@platinumazure platinumazure left a comment

Choose a reason for hiding this comment

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

Left some small comments, but otherwise this is looking great!

@mysticatea
Copy link
Member Author

I'm wondering to distinguish declare global {} blocks 🤔 (JamesHenry/typescript-estree#27)

@mysticatea
Copy link
Member Author

I'm wondering on #488.

ImportDeclarations can import type stuff, but we cannot distinguish whether the imported item is a variable or a type. So I cannot ignore types on ImportDeclaration node 🤔

@mysticatea
Copy link
Member Author

I have done except about #488.

@mysticatea
Copy link
Member Author

mysticatea commented Nov 8, 2018

I hope to separate PR for #488.

Please review!

@JamesHenry
Copy link
Member

I'll take a look at this tonight, thanks a lot @mysticatea!

@JamesHenry
Copy link
Member

JamesHenry commented Nov 9, 2018

Amazing work, @mysticatea! I just had the one question about the tests left above. I think your approach to the serialization is reasonable, although you can also create a custom matcher for jest as an alternative: https://jestjs.io/docs/en/expect#custom-matchers-api

@JamesHenry
Copy link
Member

I don't know if you noticed the pattern I established for running integration tests within docker containers in this project.

With so many potentially closed issues here, it might be nice to add them in as one or more integration tests? Let me know what you think and if you need any help with that

@mysticatea
Copy link
Member Author

🤔 I need time a bit to understand how those integration tests work since I'm not familiar with docker (and I'm on Windows 7).

@JamesHenry
Copy link
Member

Ok no problem regarding the integration tests, I can always add them in a follow up

@mysticatea
Copy link
Member Author

mysticatea commented Nov 9, 2018

Thank you.

I removed assert().

@lvivski
Copy link

lvivski commented Nov 12, 2018

Why is it still blocked? It would be great to have this PR merged. Right now we have a add a ton of exceptions in config because of all the false-positives

@platinumazure
Copy link
Member

Hi @ lvivski, there are a couple of people who still need to review this, including me.

This is open source and we all work as volunteers. We sometimes have other obligations in our lives. I'm sorry that this sometimes results in inconvenience for users who are eagerly waiting for a feature to get merged.

@lvivski
Copy link

lvivski commented Nov 12, 2018

@platinumazure sorry, I didn't mean to blame anyone but rather wanted to understand if there's anything that I can help with. Obviously everyone has life outside of github and their own obligations. I just wasn't sure what was the actual reason of blocking the merge. Unfortunately I don't have enough experience with eslint internals, but I'll try to understand it.

@platinumazure
Copy link
Member

@lvivski Thank you for your understanding. I apologize for misinterpreting your previous comment.

I've just taken a look and I only need to double-check one thing before I can leave my review-- hopefully I'll be able to approve after that check and then we can get this in soon!

Copy link
Member

@platinumazure platinumazure left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@lvivski
Copy link

lvivski commented Nov 12, 2018

Thank you so much for taking a look into it!

@mysticatea
Copy link
Member Author

@platinumazure Thank you very much!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.