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

Update to v1 and modify Roles to match BIP-003 #171

Merged
merged 8 commits into from
Sep 14, 2017
Merged

Update to v1 and modify Roles to match BIP-003 #171

merged 8 commits into from
Sep 14, 2017

Conversation

abeaumont
Copy link
Contributor

I've added minimal documentation to keep it generic, since it's hard to foresee how roles could be applied to different contexts at this point, but this should be improved as we implement support for them and become more knowledgeable, I think.

// The IfElse node is optional. The order of IfCondition, IfBody and
// IfElse is not defined.
// The Else node is optional. The order of Condition, Then and
// Else is not defined.
Copy link
Contributor

@juanjux juanjux Sep 14, 2017

Choose a reason for hiding this comment

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

Unrelated: these kind of comments with easy examples of applicability are great. We probably should improve on this on the doc page for creating drivers, with several examples of roles applies for different typical constructs in pseudocode.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, it's difficult to do so without loss of generality though.

uast/uast.go Outdated

// Update is
Copy link
Contributor

Choose a reason for hiding this comment

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

Forgot to end the sentence?

uast/uast.go Outdated
Import

// Path is a qualified name of some construct
Path
Copy link
Contributor

Choose a reason for hiding this comment

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

By the compiler warnings, this could be being overwrited by the Path in line 520.

@juanjux
Copy link
Contributor

juanjux commented Sep 14, 2017

Travis is failing®, looks like that you need to update utils.go too with the new roles.

@abeaumont
Copy link
Contributor Author

@juanjux yes, it was not ready to be merged, it contained only changes to the roles themselves, sorry.
Fixed now and included changes from review's feedback.

@juanjux
Copy link
Contributor

juanjux commented Sep 14, 2017

I understand it's ready to merge now?

@codecov
Copy link

codecov bot commented Sep 14, 2017

Codecov Report

Merging #171 into master will increase coverage by 0.05%.
The diff coverage is 76%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #171      +/-   ##
==========================================
+ Coverage   37.72%   37.77%   +0.05%     
==========================================
  Files          21       21              
  Lines        2399     2401       +2     
==========================================
+ Hits          905      907       +2     
  Misses       1406     1406              
  Partials       88       88
Impacted Files Coverage Δ
protocol/native/native.go 35.71% <ø> (ø) ⬆️
protocol/driver/positions.go 74.35% <ø> (ø) ⬆️
uast/uast.go 31.42% <ø> (ø) ⬆️
protocol/native/objecttonoder.go 64.38% <ø> (ø) ⬆️
protocol/protocol.go 25.8% <ø> (ø) ⬆️
protocol/driver/client.go 100% <ø> (ø) ⬆️
uast/role_string.go 0% <ø> (ø) ⬆️
protocol/driver/parser.go 77.77% <ø> (ø) ⬆️
protocol/driver/driver.go 35% <ø> (ø) ⬆️
protocol/driver/server.go 83.78% <ø> (ø) ⬆️
... and 4 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 63a5c96...17a30ba. Read the comment docs.

@abeaumont
Copy link
Contributor Author

Right.

@juanjux juanjux merged commit b04b0f3 into bblfsh:master Sep 14, 2017
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

2 participants