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

Add libuast iterators support#66

Merged
juanjux merged 6 commits into
bblfsh:masterfrom
juanjux:feature/iterator
Dec 22, 2017
Merged

Add libuast iterators support#66
juanjux merged 6 commits into
bblfsh:masterfrom
juanjux:feature/iterator

Conversation

@juanjux
Copy link
Copy Markdown
Contributor

@juanjux juanjux commented Dec 22, 2017

Fixes #62.

Note: CI will fail until the libuast functions' PR have been merged and tagged (I used 1.6.0 in this PR, but could update) because of the change of int to size_t in nodeiface **Size interface functions.

Juanjo Alvarez added 3 commits December 22, 2017 12:26
Signed-off-by: Juanjo Alvarez <juanjo@sourced.tech>
Signed-off-by: Juanjo Alvarez <juanjo@sourced.tech>
Signed-off-by: Juanjo Alvarez <juanjo@sourced.tech>
@juanjux juanjux self-assigned this Dec 22, 2017
@juanjux juanjux requested a review from abeaumont December 22, 2017 11:28
Copy link
Copy Markdown
Contributor

@abeaumont abeaumont left a comment

Choose a reason for hiding this comment

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

I've created libuast 1.6.0 release.
I miss support for typed filtering functions. Not sure if those will be implemented in another PR.

Comment thread bblfsh/__init__.py Outdated
from bblfsh.aliases import *

class TreeOrder:
pre_order = 0
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes, that's right for separate constants, but it's pretty common that enum-like constructs are done with classes like here, named tuples or the enum in Python 3.4 (but I didn't want to use it yet to break compatibility with older 3.x versions). Python 3.7 will introduce another one ("data classes"). It's not very important anyway, but I think the class with static members option keep things tidier than separate constants.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Well, I was mostly referring to the written in all capital letters with underscores separating words. Examples include MAX_OVERFLOW and TOTAL. part ;)

Copy link
Copy Markdown
Contributor Author

@juanjux juanjux Dec 22, 2017

Choose a reason for hiding this comment

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

I was thinking you were speaking about the class 🤦‍♂️ yes, they better should be uppercase, I'll do it.

Comment thread README.md

# You can also iterate on several tree iteration orders:
it = bblfsh.iterator(uast, bblfsh.TreeOrder.pre_order)
for node in it:
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think something like this would be more idiomatic, not sure how more difficult this would be to implement:

for node in uast.iter(bblfsh.TreeOrder.PRE_ORDER):

Copy link
Copy Markdown
Contributor Author

@juanjux juanjux Dec 22, 2017

Choose a reason for hiding this comment

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

I checked a while ago (when I did something similar for the Scala client) and it was not trivial, so I would leave that for another PR (including also uast.filter).

Comment thread bblfsh/pyuast.c
return NULL;
}

pyIt->iter = UastIteratorNew(ctx, node, (TreeOrder)order);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Check for null missing either here or in the caller code

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

juanjux commented Dec 22, 2017

Yes, my idea was to add the filtering functions in another PR after the Scala and Go clients have the iterators too.

Comment thread bblfsh/pyuast.c Outdated
}

pyIt->iter = UastIteratorNew(ctx, node, (TreeOrder)order);
if (!pyIt->iter)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I guess a Py_DECREF(pyIt); is also needed here

Juanjo Alvarez added 2 commits December 22, 2017 13:55
Signed-off-by: Juanjo Alvarez <juanjo@sourced.tech>
Signed-off-by: Juanjo Alvarez <juanjo@sourced.tech>
@juanjux juanjux merged commit 0a914d6 into bblfsh:master Dec 22, 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.

2 participants