Skip to content
This repository has been archived by the owner on Mar 8, 2020. It is now read-only.

Preserve raw tokens for strings and regexps #50

Merged
merged 2 commits into from
Jan 22, 2019
Merged

Conversation

dennwc
Copy link
Member

@dennwc dennwc commented Jan 3, 2019

Currently, JS driver uses unescaped string and regexp literals from the native AST as @token. This leads to confusion for ML, since they expect string token to cover the whole node in the source code (according to the positional information).

Although in general, this will require adding a separate "token stream" to the UAST, those two issues can be solved in the current version.

See comments in related issues for details (link is in the commit message).

@dennwc dennwc self-assigned this Jan 3, 2019
Signed-off-by: Denys Smirnov <denys@sourced.tech>
@bzz
Copy link
Contributor

bzz commented Jan 9, 2019

👍 If I understood the change, it boils down to keeping whole literals in @token:

Code: string literal attribute="If parent is JSX keep double quote"
Before

  • Node @token If parent is JSX keep double quote
    After
  • Node @token "If parent is JSX keep double quote"

and same for
Code: regexp literal /a/g
Before

@dennwc could you please help me understand, how/if we want it to be consistent with behavior of other drivers?

On

See comments in related issues for details.

not sure which issue is that.

On driver/normalizer/strconv.go- would it make sense to have simple tests for those functions?

@dennwc
Copy link
Member Author

dennwc commented Jan 9, 2019

@bzz Other drivers already include quotes in @token for Native and Annotated mode. This is only an issue with the JS driver that uses wrong fields from native AST.

As for strconv.go, I don't think it requires extensive test coverage since it's copied from the Go stdlib, which already ensures that the code is correct. But we can add a few small test cases to detect regressions.

@bzz
Copy link
Contributor

bzz commented Jan 9, 2019

Thank you for kind explanations!

On

See comments in related issues for details.

not sure which issue is that.

Have missed those in commit messages - #22 and #32

Copy link
Contributor

@bzz bzz left a comment

Choose a reason for hiding this comment

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

👏

driver/normalizer/normalizer.go Outdated Show resolved Hide resolved
Signed-off-by: Denys Smirnov <denys@sourced.tech>
@dennwc dennwc merged commit ecdce1f into bblfsh:master Jan 22, 2019
@dennwc dennwc deleted the string_lit branch January 22, 2019 16:05
@dennwc dennwc mentioned this pull request Feb 5, 2019
36 tasks
@bzz bzz mentioned this pull request Mar 13, 2019
4 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants