Skip to content

fix: respect herecomments when computing function bounds#162

Merged
alangpierce merged 2 commits intodecaffeinate:masterfrom
alangpierce:respect-herecomment-bounds
Feb 19, 2017
Merged

fix: respect herecomments when computing function bounds#162
alangpierce merged 2 commits intodecaffeinate:masterfrom
alangpierce:respect-herecomment-bounds

Conversation

@alangpierce
Copy link
Member

Fixes decaffeinate/decaffeinate#687

Previously, the bounds of a function body were shrunk to only include semantic
tokens, which could cause a block comment to get moved outside the body and be
improperly indented after the normalize step. Now, we include the comments as
part of the body.

This is a small change that gets this case to work, but it might be that in the
future we'll want to avoid all AST location adjustment and/or put block comments
in the AST.

Fixes decaffeinate/decaffeinate#687

Previously, the bounds of a function body were shrunk to only include semantic
tokens, which could cause a block comment to get moved outside the body and be
improperly indented after the normalize step. Now, we include the comments as
part of the body.

This is a small change that gets this case to work, but it might be that in the
future we'll want to avoid all AST location adjustment and/or put block comments
in the AST.
Copy link
Collaborator

@eventualbuddha eventualbuddha left a comment

Choose a reason for hiding this comment

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

What happens when a block comment follows a function body but isn't indented to match?

@alangpierce
Copy link
Member Author

I think the block comment is still viewed as outside the function, since the CoffeeScript parser doesn't include it in the function bounds in the first place. This code path that I'm changing is just shrinking the location data from the CoffeeScript parser, but if the CoffeeScript parser got everything correct, I think it wouldn't be necessary.

But I can add a test to double-check.

Also change the test name from "herecomment" to "block comment" to be consistent
with other tests/code.
@alangpierce alangpierce merged commit 898a759 into decaffeinate:master Feb 19, 2017
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.

2 participants

Comments