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

Conversation

juanjux
Copy link
Contributor

@juanjux juanjux commented Jun 5, 2018

Pending:

  • Column positions seem to be off. Odd because they've the same value in the native AST, but the previous version fixed it somewhat, so I need to copy that "somewhat". (Fixed: the old driver computes the line and col from the offset ignoring the usually wrong native ones. Changed it to do the same).
  • Empty attributes node (positions are there, are translated but the node remains). Not critical but it's ugly.
  • Name.parts doesn't get the Unannotated roles and doesn't get new types set. Seems to be an SDK problem, but I'll join them into a single string like the previous driver did.
  • File missing on root, even trough I added it directly in the native AST (Fixed: had to delete the build/ directory to get the image to update).
  • CI is failing.

@juanjux juanjux requested a review from dennwc June 5, 2018 11:26
@juanjux juanjux self-assigned this Jun 5, 2018
@juanjux juanjux changed the title [WIP] Bip 5 Bip 5 Jun 5, 2018
@juanjux
Copy link
Contributor Author

juanjux commented Jun 5, 2018

@dennwc PTAL when you can, all remaining issues should have been fixed.

Copy link
Member

@dennwc dennwc left a comment

Choose a reason for hiding this comment

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

Awesome! Few micro changes and let's merge it! :)

"gopkg.in/bblfsh/sdk.v2/sdk/driver/fixtures"
)

//const projectRoot = "/opt/driver/src"
Copy link
Member

Choose a reason for hiding this comment

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

please remove the comment

On(HasInternalRole("cond")).Roles(uast.Case),
),
func (op opParts2Str) Check(st *State, n uast.Node) (bool, error) {
if _, ok := n.(uast.String); ok {
Copy link
Member

Choose a reason for hiding this comment

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

This check is redundant, since n.(uast.Array) already filters this case

),
),
func (op opParts2Str) Construct(st *State, n uast.Node) (uast.Node, error) {
n, err := op.orig.Construct(st, n)
Copy link
Member

Choose a reason for hiding this comment

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

just return op.orig.Construct(st, n)

@dennwc
Copy link
Member

dennwc commented Jun 6, 2018

LGTM! Feel free to squash/merge as necessary.

- Some HasInternalRole conversions

- Assignmnent and opAssign annotations

- Added phpast aliases module

- More annotations. Reparent native attributes

- Some conditional annotations

- Some fixes

- UncommentCLike working, update to SDK.v2

- Add missing files

- Comment positions

- Added missing tokens

- Delete old tonoder test

Signed-off-by: Juanjo Alvarez <[email protected]>

Fixed build with v2

fix name transform

Signed-off-by: Denys Smirnov <[email protected]>

Cleanup of debug stuff

Signed-off-by: Juanjo Alvarez <[email protected]>

Remove debug yaml output

Signed-off-by: Juanjo Alvarez <[email protected]>

Fix Makefile

Signed-off-by: Juanjo Alvarez <[email protected]>

Fix some unannotated roles

Signed-off-by: Juanjo Alvarez <[email protected]>

xxx

Signed-off-by: Juanjo Alvarez <[email protected]>

Fix several issues

Signed-off-by: Juanjo Alvarez <[email protected]>

Compute line and col from offset instead of using the native ones

Signed-off-by: Juanjo Alvarez <[email protected]>

Remove the now unneeded attributes object

Signed-off-by: Juanjo Alvarez <[email protected]>

Join Name.parts into a single string

Signed-off-by: Juanjo Alvarez <[email protected]>

Fix CI

Signed-off-by: Juanjo Alvarez <[email protected]>

Fix unannotated name corner case

Signed-off-by: Juanjo Alvarez <[email protected]>

Fixes from review

Signed-off-by: Juanjo Alvarez <[email protected]>
@juanjux juanjux merged commit e53bf5e into bblfsh:master Jun 6, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants