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

Add missing remaining flow annotations #27

Merged
merged 4 commits into from
Aug 9, 2018
Merged

Add missing remaining flow annotations #27

merged 4 commits into from
Aug 9, 2018

Conversation

juanjux
Copy link
Contributor

@juanjux juanjux commented Aug 8, 2018

Missed a few ones in the last PR.

Related to: #28, #24.

Signed-off-by: Juanjo Alvarez juanjo@sourced.tech

Signed-off-by: Juanjo Alvarez <juanjo@sourced.tech>
@juanjux juanjux self-assigned this Aug 8, 2018
@juanjux juanjux requested a review from dennwc August 8, 2018 16:37
@dennwc
Copy link
Member

dennwc commented Aug 8, 2018

Can you please add them to tests as well?

…TypeProperty

Signed-off-by: Juanjo Alvarez <juanjo@sourced.tech>
@juanjux
Copy link
Contributor Author

juanjux commented Aug 9, 2018

@dennwc test improved to cover these too.

Signed-off-by: Juanjo Alvarez <juanjo@sourced.tech>
@juanjux juanjux mentioned this pull request Aug 9, 2018
Signed-off-by: Juanjo Alvarez <juanjo@sourced.tech>
@juanjux
Copy link
Contributor Author

juanjux commented Aug 9, 2018

Usually what should done when creating a driver is get all the possible types from the native driver source code and ensure that the tests cover all of them. Looks like when the initial tests of this driver were created that wasn't done, I'll add a separate issue.

col: 19,
},
},
Name: "testfnc_object",
Copy link
Member

Choose a reason for hiding this comment

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

I can't find annotations in the semantic tree, it seems like some transformation stage cuts them out.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, should I open an issue?

Copy link
Member

Choose a reason for hiding this comment

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

It exists in the native tree, so something is wrong with current annotations. Do you want to open a separate issue/PR for it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you check the PR the annotations I added were really pretty simple of the "just add roles to these types" kind. Probably we should just close this PR since it's incomplete anyway because of #28.

Copy link
Member

Choose a reason for hiding this comment

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

Agree, if by "close" you mean "merge"

Copy link
Contributor Author

@juanjux juanjux Aug 9, 2018

Choose a reason for hiding this comment

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

Yeah, can be merged (more annotations and test wont hurt) but removing the "Fixes" since as soon as it's merged @EgorBu will find some other Unannotated from some other file until we fix #28.

Edit: s/can't/can

@dennwc dennwc merged commit 81d3654 into bblfsh:master Aug 9, 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.

None yet

2 participants