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

Fix #365 #425

Closed
wants to merge 12 commits into from
Closed

Fix #365 #425

wants to merge 12 commits into from

Conversation

lucivpav
Copy link

@lucivpav lucivpav commented Oct 7, 2020

Fixes issue #365 (maybe more).

Changes:

  • fix of processing of parenthesized expressions that start with comments
  • add tests
  • the trailing comments are still the same (incorrect placement), but they do not affect the syntax (can be addressed in a future PR)
  • blank line preservation: seems no new problems with this

@lucivpav
Copy link
Author

lucivpav commented Oct 7, 2020

@micschro

@lucivpav
Copy link
Author

lucivpav commented Oct 13, 2020

arrow functions are affected too! #426 (comment)
Solved.

@lucivpav
Copy link
Author

Maybe these kind of bugs should be treated from another point of view - if a parenthesis expression contains comments, then the parenthesis must stay.

@Meir017
Copy link

Meir017 commented Oct 13, 2020

@lucivpav the new test is failing...

image

@lucivpav
Copy link
Author

It is failing on purpose, because I did not implement a fix of arrow functions in this PR yet :)

@Meir017
Copy link

Meir017 commented Oct 15, 2020

another scenario that's related to broken parens

(1).toString()

is generated as

1.toString()

which is invalid js :(

@papandreou
Copy link
Contributor

1.toString()

Not sure if you're saying that's broken on this branch or in the latest release, but it's not reproducible on master:

> require('escodegen').generate({ type: 'CallExpression', callee: { type: 'MemberExpression', object: { type: 'Literal', value: 1 }, property: { type: 'Identifier', name: 'toString' }, computed: false, optional: false }, arguments: [], optional: false })
'1 .toString()'

@Meir017
Copy link

Meir017 commented Oct 15, 2020

1.toString()

Not sure if you're saying that's broken on this branch or in the latest release, but it's not reproducible on master:

require('escodegen').generate({ type: 'CallExpression', callee: { type: 'MemberExpression', object: { type: 'Literal', value: 1 }, property: { type: 'Identifier', name: 'toString' }, computed: false, optional: false }, arguments: [], optional: false })
'1 .toString()'

the output should wrap the Literal number with parens, running the generated code fails with a syntax error

image

@papandreou
Copy link
Contributor

Did you notice the space between . and toString? That makes it run.

@Meir017
Copy link

Meir017 commented Oct 15, 2020

@papandreou my bad.
although I still think this is incorrect behavior is incorrect but it's not very important

@papandreou
Copy link
Contributor

You're of course entitled to your opinion, but why do you think it's incorrect? ;)

@Meir017
Copy link

Meir017 commented Oct 15, 2020

the behavior isn't consistent.
for example:

(1).toString(); // outputs "1"
(1+2).toString(); // outputs "3"
1 + 2 .toString(); // outputs "12"

is converted to

1 .toString(); // outputs "1"
(1 + 2).toString(); // outputs "3"
1 + 2 .toString(); // outputs "12"

maybe this is just a js thing that is confusing...

@lucivpav lucivpav marked this pull request as draft October 15, 2020 19:59
@papandreou
Copy link
Contributor

maybe this is just a js thing that is confusing...

Yeah, I think those examples are JavaScript being weird more than escodegen :)

@lucivpav lucivpav marked this pull request as ready for review October 19, 2020 19:29
@lucivpav
Copy link
Author

@michaelficarra could you please take a look at this PR, when you have time?

@lucivpav
Copy link
Author

Do not merge as it is now. I discovered a bug in cases like these:

class MyClass {
    /**
     * description
     */
    foo(a, b) {
        a.bar(b);
    }
}

@lucivpav lucivpav closed this Nov 25, 2020
@lucivpav lucivpav mentioned this pull request Dec 9, 2020
@lucivpav lucivpav mentioned this pull request Dec 21, 2020
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.

3 participants