Fix: Exception thrown when space occurs after function name #124

soda0289 · 2016-12-08T21:44:50Z

Possible fix for #123

When parsing typescript node we need to add an offset when there is a space between function name and parameters.

Still need to add tests. Would like to know if I'm on the right track.

jsf-clabot · 2016-12-08T21:44:51Z

All committers have signed the CLA.

eslintbot · 2016-12-08T21:44:52Z

Thanks for the pull request, @soda0289! 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.)

JamesHenry

Thanks for contributing @soda0289!

Please could you address the following before we get this merged:

See my comment inline for a tweak to the logic
Add a test in test/fixtures/ecma-features/classes and use your example class from TypeError: Cannot read property 'value' of undefined - When parsing with rule space-before-function-paren #123 as the src
Remove the .swp file from the commit

JamesHenry · 2016-12-13T22:43:13Z

lib/ast-converter.js

@@ -1048,6 +1048,7 @@ module.exports = function(ast, extra) {
            case SyntaxKind.GetAccessor:
            case SyntaxKind.SetAccessor:
            case SyntaxKind.MethodDeclaration:
+                var offset = node.parameters.pos - node.name.end - 1;


This calculation ends up containing both +node.name.end and -node.name.end so we could simplify the range start to just be node.parameters.pos - 1, and scrap this variable altogether right?

As an aside, most of the conversion currently happens within one single function scope right now, so please give try and give variables more specific and unique names. I will be refactoring at some point once the project has fully stabilised, but just something to bear in mind at the moment.

eslintbot · 2016-12-22T02:30:31Z

Thanks for the pull request, @soda0289! 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.)

eslintbot · 2016-12-22T03:11:21Z

Thanks for the pull request, @soda0289! 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.)

soda0289 · 2016-12-22T03:13:03Z

@JamesHenry
Thanks for the review. I got rid of the offset variable and just use node.parameters.pos now. I added a test and got it to pass. I noticed lots of other tests are still failing and might spend some time looking into.

JamesHenry · 2017-01-01T15:53:49Z

Thanks a lot for your patience on this, @soda0289! As soon as #131 gets merged we can see if rebasing this branch fixes the build

JamesHenry · 2017-01-01T16:04:30Z

It's in! Please rebase the PR and we'll see where we're at 😄

eslintbot · 2017-01-04T01:24:18Z

Thanks for the pull request, @soda0289! 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.)

eslintbot · 2017-01-04T01:25:38Z

Thanks for the pull request, @soda0289! 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.)

soda0289 · 2017-01-04T14:48:00Z

@JamesHenry
I reabsed off of the updated master but I can't seem to get eslintbot to be satisfied. I tried changing the last line of the commit message to fixes #123 but it still complains.

Let me know if you want anything else changed.

JamesHenry · 2017-01-04T15:02:58Z

It is validating against our contributing guidelines.

Relevant excerpt:

The message summary should be a one-sentence description of the change, and it must be 72 characters in length or shorter. If the pull request addresses an issue, then the issue number should be mentioned at the end. If the commit doesn’t completely fix the issue, then use (refs #1234) instead of (fixes #1234).

Here are some good commit message summary examples:

Fix: Semi rule incorrectly flagging extra semicolon (fixes #840)

Well, this change is currently breaking a lot of existing tests (causing the build to fail), so the next job will be to look into those and double-check whether or not that should be the case, i.e. do the tests need updating, or does the code change? Regardless, the PR will only be merged once we get to a point where the build is passing.

soda0289 · 2017-01-04T15:07:24Z

I see the test are failing because some of the positions are off in range array. Will look at this in a couple hours. Thanks.

…lint#123) We can calculate where the function parameters starts by using node.parameters.pos and adding one.

eslintbot · 2017-01-04T17:48:29Z

LGTM

soda0289 · 2017-01-04T17:49:48Z

@JamesHenry
I used plus one instead of minus one. Also one test had to be fixed. The test used spaces after function names and had a different range number.

soda0289 · 2017-01-04T19:39:51Z

Can you ensure that the range values are correct for the test I wrote and the one I changed?

JamesHenry · 2017-01-04T22:35:51Z

LGTM, thanks!

…lint#123) (eslint#124)

soda0289 changed the title ~~Fix error with space after function name~~ Fix: Exception thrown when space occurs after function name Dec 9, 2016

JamesHenry suggested changes Dec 13, 2016

View reviewed changes

soda0289 force-pushed the fix-space-func-parens branch from c204b8d to ff9d21a Compare December 22, 2016 02:30

soda0289 force-pushed the fix-space-func-parens branch from ff9d21a to 415299f Compare December 22, 2016 03:11

soda0289 force-pushed the fix-space-func-parens branch from 415299f to 9eb8f5f Compare January 4, 2017 01:24

soda0289 force-pushed the fix-space-func-parens branch from 9eb8f5f to 82ed70c Compare January 4, 2017 01:25

JamesHenry mentioned this pull request Jan 4, 2017

Add async/await support #129

Merged

Fix: Exception thrown when space occurs after function name (fixes es…

a8501eb

…lint#123) We can calculate where the function parameters starts by using node.parameters.pos and adding one.

soda0289 force-pushed the fix-space-func-parens branch from 82ed70c to a8501eb Compare January 4, 2017 17:48

JamesHenry approved these changes Jan 4, 2017

View reviewed changes

JamesHenry merged commit 0ff19dd into eslint:master Jan 4, 2017

soda0289 added a commit to soda0289/typescript-eslint-parser that referenced this pull request Jan 9, 2017

Fix: Exception thrown when space occurs after function name (fixes es…

5431a1b

…lint#123) (eslint#124)

soda0289 deleted the fix-space-func-parens branch May 23, 2017 13:27

Uh oh!

Fix: Exception thrown when space occurs after function name #124

Fix: Exception thrown when space occurs after function name #124

Uh oh!

Conversation

soda0289 commented Dec 8, 2016

Uh oh!

jsf-clabot commented Dec 8, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

eslintbot commented Dec 8, 2016

Uh oh!

JamesHenry left a comment

Choose a reason for hiding this comment

Uh oh!

JamesHenry Dec 13, 2016

Choose a reason for hiding this comment

Uh oh!

eslintbot commented Dec 22, 2016

Uh oh!

eslintbot commented Dec 22, 2016

Uh oh!

soda0289 commented Dec 22, 2016

Uh oh!

JamesHenry commented Jan 1, 2017

Uh oh!

JamesHenry commented Jan 1, 2017

Uh oh!

eslintbot commented Jan 4, 2017

Uh oh!

eslintbot commented Jan 4, 2017

Uh oh!

soda0289 commented Jan 4, 2017

Uh oh!

JamesHenry commented Jan 4, 2017

Uh oh!

soda0289 commented Jan 4, 2017

Uh oh!

eslintbot commented Jan 4, 2017

Uh oh!

soda0289 commented Jan 4, 2017

Uh oh!

soda0289 commented Jan 4, 2017

Uh oh!

JamesHenry commented Jan 4, 2017

Uh oh!

Uh oh!

jsf-clabot commented Dec 8, 2016 •

edited

Loading