-
Notifications
You must be signed in to change notification settings - Fork 21
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add new test for _parse_file #181
Conversation
realted to bblfsh/javascript-driver#36 |
ffeac8d
to
849e642
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍! Does it also fix #180?
@@ -25,6 +25,15 @@ def setUpClass(cls): | |||
cls.uast = bblfsh.Node.FromString(fin.read()) | |||
cls.extractor = FeatureExtractor("javascript", parents_depth=2, siblings_window=5) | |||
|
|||
def test_parse_file2(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please rename to something more informative like test_parse_file_on_comments if comments are causing a problem for example.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mmm, it is not only for comments. It is for random stuff. I rename it to test_parse_file_exact_match
with lzma.open(str(test_js_code_filepath), mode="rt") as f: | ||
code = f.read() | ||
uast = bblfsh.BblfshClient("0.0.0.0:9432").parse( | ||
filename="", language="javascript", contents=code.encode()).uast |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no need to specify filename when contents is set. It could also be good to just bypass babelfish and just use a saved uast as we do in the other tests. Up to you :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nope. here the header of parse function: def parse(self, filename, language=None, contents=None, timeout=None):
also the file name goes to bblfsh logs.
When we update to the next version of a bblfsh driver, we have to update this files.
I prefer to have it like that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair enough, even though we will have to update all the other files in the folder anyway. And huh, this function signature needs refactoring, I'll open an issue.
Signed-off-by: konstantin <[email protected]>
No, I check it and it does not fix #180 |
"".join(n.value for n in nodes)
wherenodes, _ = self.extractor._parse_file(...)
produce bad result.here is the diff from added test: