add optional SourceLocation to Node interface #7

Merged
merged 1 commit into from Dec 29, 2014

Conversation

Projects
None yet
2 participants
@michaelficarra
Member

michaelficarra commented Dec 25, 2014

/cc @josh

@josh

This comment has been minimized.

Show comment
Hide comment
@josh

josh Dec 25, 2014

nice.

I guess Comment nodes are the only thing left on my 🎄 wishlist 😉

josh commented Dec 25, 2014

nice.

I guess Comment nodes are the only thing left on my 🎄 wishlist 😉

@michaelficarra

This comment has been minimized.

Show comment
Hide comment
@michaelficarra

michaelficarra Dec 27, 2014

Member

Quoting myself from Twitter:

Unlikely to be attached to the AST. Everyone wants different placement. A parser is free to output side tables with CST info.

I strongly feel that it is not the parser's job to determine the node to which a comment gets attached. In recent conversations with @ariya, he has agreed. Esprima bundling comment attachment, choosing an algorithm for you, was a bad idea. I believe we should leave the decision of how to handle this up to the parsers themselves, and I recommend they simply expose the insignificant syntax in a side-table (or two) along with their source offsets. Then consumers can handle the association themselves.

Member

michaelficarra commented Dec 27, 2014

Quoting myself from Twitter:

Unlikely to be attached to the AST. Everyone wants different placement. A parser is free to output side tables with CST info.

I strongly feel that it is not the parser's job to determine the node to which a comment gets attached. In recent conversations with @ariya, he has agreed. Esprima bundling comment attachment, choosing an algorithm for you, was a bad idea. I believe we should leave the decision of how to handle this up to the parsers themselves, and I recommend they simply expose the insignificant syntax in a side-table (or two) along with their source offsets. Then consumers can handle the association themselves.

@michaelficarra michaelficarra referenced this pull request in shapesecurity/shift-codegen-js Dec 27, 2014

Open

Source maps #3

@michaelficarra

This comment has been minimized.

Show comment
Hide comment
@michaelficarra

michaelficarra Dec 29, 2014

Member

Merging, since there have been no objections and I want to get started adding this feature to the parser.

Member

michaelficarra commented Dec 29, 2014

Merging, since there have been no objections and I want to get started adding this feature to the parser.

michaelficarra added a commit that referenced this pull request Dec 29, 2014

Merge pull request #7 from shapesecurity/source-location
add optional SourceLocation to Node interface

@michaelficarra michaelficarra merged commit 042ed27 into master Dec 29, 2014

@michaelficarra michaelficarra referenced this pull request in shapesecurity/shift-parser-js Jan 8, 2015

Closed

index-based range #10

@michaelficarra michaelficarra deleted the source-location branch Jan 22, 2015

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