feat: add find api #16

manucorporat · 2017-07-24T16:25:02Z

No description provided.

juanjux · 2017-07-26T08:41:32Z

bblfsh/__main__.py

    parser = argparse.ArgumentParser(
        description="Query for a UAST to Babelfish and dump it to stdout."
    )
+    parser.add_argument('filename', type=str,


This change seems to be unrelated with the usage of libuast. Was it requested by the machine learning team?

juanjux · 2017-07-26T08:42:58Z

bblfsh/client.py

-        return response
+        return self._stub.Parse(request, timeout=timeout)
+
+    def parse_uast(self, filename, language=None, contents=None, timeout=None,


This should be def parse( as in ~master.

juanjux · 2017-07-26T08:43:33Z

bblfsh/__main__.py

-    print(client.parse(args.file, args.language))

+    client = BblfshClient(args.endpoint)
+    root = client.parse_uast(args.filename, args.language)


root = client.parse(...

juanjux · 2017-07-26T08:49:54Z

bblfsh/test.py

+        node8 = NodeNative("loop")
+
+        root.add_child(node1)
+


Spurious whitespace?, probably all the add_child statements would be better on a single logical paragraph (without whitespace between them).

juanjux · 2017-07-26T08:54:19Z

Makefile

+	cd libuast && cmake . && make && make install
+
+bblfsh/pyuast.py: libuast
+	cd bblfsh && swig -c++ -python -I/usr/local/include uast.i


So swig is needed, at least for make. Since the .i and pyuast.py files are included, couldn't we remove the all: target dependency on this one? (but keeping this in the Makefile just in case we want to regenerate pyuast.py).

juanjux

Check the failing Travis build, maybe you need to install gcc or something like that.

juanjux · 2017-08-01T13:37:03Z

bblfsh/pyuast.cpp

+  {
+    return NULL;
+  }
+  printf("BEFORE FIND!! %p\n", obj);


These things usually go inside an #ifdef DEBUG

juanjux · 2017-08-01T13:41:06Z

Travis still failing ^^^^^^^

abeaumont · 2017-08-01T16:44:46Z

bblfsh/pyuast.cpp

+#include <libuast/uast.h>
+#include <Python.h>
+
+#ifdef __cplusplus


There's no need for this file being C++, it should be a C file and these lines become unnecessary

abeaumont · 2017-08-01T16:47:13Z

bblfsh/pyuast.cpp

+extern "C" {
+#endif
+
+static const char *


No need to split these in 2 lines, it should be consistent everywhere and it's not wide enough to split it.

abeaumont · 2017-08-01T16:54:56Z

bblfsh/pyuast.cpp

@@ -0,0 +1,117 @@
+#include <libuast/uast.h>


#include <stdint.h>

abeaumont · 2017-08-02T12:58:45Z

bblfsh/pyuast.cpp

+    return NULL;
+  }
+  printf("BEFORE FIND!! %p\n", obj);
+  auto results = api.find(obj, std::string(query));


This should be C code.

abeaumont · 2017-08-02T13:36:20Z

bblfsh/__main__.py

 import argparse
 import sys

+from bblfsh._pyuast import find


I don't think pyuast should be a private module, should it?

I don't know if there is specified in PEP8 but there is an old convention in Python of using a leading underline for binary modules.

PS: forget it, it's only when the pure Python module has the same name.

abeaumont · 2017-08-02T13:40:16Z

bblfsh/client.py

                         for example "0.0.0.0:9432"
        :type endpoint: str
        """
+        Node.initialize()


I don't think a libuast Node should be initialized in a GRPC client, why is it this way?

abeaumont · 2017-08-02T13:42:40Z

bblfsh/pyuast.cpp

+
+#ifdef __cplusplus
+}
+#endif


Newline missing

abeaumont · 2017-08-08T08:07:20Z

.vscode/settings.json

@@ -0,0 +1,30 @@
+{


this file should not be here

abeaumont · 2017-08-30T13:29:26Z

bblfsh/pyuast.c

@@ -0,0 +1,119 @@
+#include "uast.h"


Is there a code style for python modules? If not, I guess we should follow Google C++ code style, which is not been followed here.

Not that I know for - or resulted from a quick search. Since @manucorporat probably won't be working on this anymore I'll change it soon.

abeaumont · 2017-08-30T13:29:54Z

bblfsh/pyuast.c

+{
+  PyObject *node = (PyObject *)data;
+  PyObject *attribute = PyObject_GetAttrString(node, prop);
+  const char *a = PyUnicode_AsUTF8(attribute);


return PyUnicode_AsUTF8(attribute)

abeaumont · 2017-08-30T13:31:35Z

bblfsh/pyuast.c

+  return PyModule_Create(&moduledef);
+}
+
+#ifdef __cplusplus


This should be removed

abeaumont · 2017-08-30T13:33:40Z

setup.py

+from setuptools import setup, find_packages, Extension

+LIBRARIES = ['xml2']
+SOURCES = ['bblfsh/pyuast.c']


These are not constants, they shouldn't be upcase

I've frequently seen some uppercased module-globals on Python code that were not constants but it doesn't seem to be a consensus, PEP8 says that constants are uppercase but doesn't say anything about module (mutable) globals but I think that making them lowercase avoids confusion, yes.

juanjux · 2017-09-06T13:11:31Z

Closing it because I'll reopen this PR from my branch - I already implemented all the suggested changes, but found a crashing bug with some parameters for find(), debugging it right now.

juanjux · 2017-09-06T13:17:34Z

To have both PRs linked, the new (unfinished) one is /pull/23

juanjux reviewed Jul 26, 2017

View reviewed changes

juanjux requested review from abeaumont and vmarkovtsev July 26, 2017 09:01

manucorporat force-pushed the add-libuast branch from 0a794ce to 617c4dc Compare July 26, 2017 09:56

feat: add xpath api

6e540f2

manucorporat force-pushed the add-libuast branch from 617c4dc to 6e540f2 Compare July 26, 2017 10:18

juanjux approved these changes Aug 1, 2017

View reviewed changes

bblfsh/pyuast.cpp Outdated

{

return NULL;

}

printf("BEFORE FIND!! %p\n", obj);

Copy link

Contributor

juanjux Aug 1, 2017

Choose a reason for hiding this comment

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

These things usually go inside an #ifdef DEBUG

abeaumont suggested changes Aug 2, 2017

View reviewed changes

manucorporat force-pushed the add-libuast branch 4 times, most recently from 353f43b to 611635f Compare August 8, 2017 16:27

using native nodes

2469036

manucorporat force-pushed the add-libuast branch from 611635f to 2469036 Compare August 8, 2017 16:28

manucorporat added 2 commits August 8, 2017 18:30

Merge branch 'master' into add-libuast

679c29a

Using tagged builds of libuast

da97ee3

manucorporat force-pushed the add-libuast branch 7 times, most recently from 6397d05 to 4b2c003 Compare August 23, 2017 14:54

manucorporat changed the title ~~[WIP] feat: add xpath api~~ feat: add find api Aug 23, 2017

manucorporat force-pushed the add-libuast branch from 4b2c003 to e55d61d Compare August 23, 2017 14:59

Merge branch 'master' into add-libuast

c26ba6f

manucorporat force-pushed the add-libuast branch from e55d61d to c26ba6f Compare August 24, 2017 11:13

abeaumont suggested changes Aug 30, 2017

View reviewed changes

juanjux closed this Sep 6, 2017

juanjux mentioned this pull request Sep 6, 2017

Add libuast, upgrade libuast API, bugfixing #23

Merged

feat: add find api #16

feat: add find api #16

Uh oh!

Conversation

manucorporat commented Jul 24, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

juanjux Jul 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

juanjux left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

juanjux commented Aug 1, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

juanjux Aug 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

juanjux Sep 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

juanjux commented Sep 6, 2017

Uh oh!

juanjux commented Sep 6, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

juanjux Jul 26, 2017 •

edited

Loading

juanjux Aug 2, 2017 •

edited

Loading

juanjux Sep 1, 2017 •

edited

Loading