Add check for node.body in referencer #2
Conversation
|
|
There was a problem hiding this comment.
Ha I had a bit of a WTF moment when I came to submit my PR, and you had already done it. Apparently you see everything 👀
The only thing I did differently, and would request that you do, is add a comment as to why we are adding the check:
// In TypeScript there are a number of function-like constructs which have no body,
// so check it exists before traversing
if (node.body) {
|
@JamesHenry hah yes sorry I am a lurker have been wanting to help more, but you know work :/ figured this one took me all of 5 min :) Should be all set! |
|
@nzakas Sure may take me some digging but once the rest of my work is done today will try to take a crack at it. |
|
@JamesHenry I am going to be super busy till the weekend. If you want to do this instead before then is totally fine with me :) |
|
@corbinu any update on this? |
|
@JamesHenry sorry work has gotten away from me. I should have some time thrusday but if somebody else wants to pickup the tests is fine :) |
|
I don't mind writing tests. Would love to help. I'll take a shot at it later today. |
|
@soda0289 Sure thanks that would really help! |
We will remove this soon but need to add tests for new features in the mean time.
This used in tests for typescript features.
This fixes an issue with typescript having an empty body for some functions
|
I added some tests for typescript. Pull request can be found here: I dont think the scope is correct. For example the abstract class is not found in the scope. I think the other test, for multiple function declartion, are correct just need to double check. I also had to add a check to scope.js to return false when body is not found in the function isStrictScope(). |
|
Thanks will add them to mine. |
This includes a test for multiple call signatures.
Node body check - with tests
|
@JamesHenry @nzakas @soda0289 Was nice enough to get some tests working this should be good to go. @soda0289 Is going to do another PR that adds changes and tests to eslint-scope to properly scope Typescript Abstract classes. |
|
Thanks so much for your efforts on this guys! I was never a contributor to escope, and I do not feel I am in a good position to be able to pass judgement on how well these test cases match expectations. The code itself certainly seems fine 👍 @nzakas Requested them originally, and certainly has been an important contributor to escope in the past, so his review here is key. @corbinu The main thing from me is to make sure that this branch is up to date, there are currently conflicts |
# Conflicts: # package.json
|
@JamesHenry should be all set! |
Add comment explaining the node.body check Allow tests to run with babel We will remove this soon but need to add tests for new features in the mean time. Add typescript-eslint-parser as dev dependency This used in tests for typescript features. Strict scope should be false when there is no body This fixes an issue with typescript having an empty body for some functions Add tests for typescript scope analysis This includes a test for multiple call signatures. Add comment explaining the node.body check Allow tests to run with babel We will remove this soon but need to add tests for new features in the mean time. Strict scope should be false when there is no body This fixes an issue with typescript having an empty body for some functions Add tests for typescript scope analysis This includes a test for multiple call signatures.
|
@JamesHenry Sorry if I jacked the rebase is that better? |
|
Hmm, it's not letting me merge for some reason. Will try again later. @corbinu thanks for your help. In the future, please be sure to follow our commit message guidelines. It helps the process be a bit smoother. |
ilyavolodin
dismissed
JamesHenry’s stale review
All requests have been addressed.
|
@nzakas Merge was disabled because @JamesHenry have requested changes and review was not approved. I'll merge this in as "Update" |
|
👍 |
|
Ah weird, that request for changes didn't show up in the mobile view. Thanks! |
So happy to make this PR!! Obviously this fixes issues for scoping typescript methods without a body per eslint/typescript-eslint-parser#92