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

Conversation

@quinor
Copy link

@quinor quinor commented Sep 21, 2018

still WIP, diffing works, applying diff not implemented yet

keys := make(map[string]bool)
iterate := func(keyset nodes.Object) {
for key := range keyset {
if in := keys[key]; !in {
Copy link
Member

Choose a reason for hiding this comment

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

This comment still applies

@dennwc
Copy link
Member

dennwc commented Sep 24, 2018

Also, it would be nice to have some tests for diff generation as well as for appling the diff.

@quinor
Copy link
Author

quinor commented Oct 27, 2018

How do we add tests in the repository? I've got some internal ones.

@dennwc
Copy link
Member

dennwc commented Oct 30, 2018

@quinor You can add a test function to a new file ./uast/diff/diff_test.go. CI will call it automatically.

@quinor
Copy link
Author

quinor commented Oct 30, 2018

@dennwc how to proceed if the test(s) need serialized data or external resources to work? My current testing procedure is to launch a client that runs the diff on two files and tries to apply it afterwards comparing the result with source, additionally having bblfshd docker running for the source file parsing.

@dennwc
Copy link
Member

dennwc commented Oct 30, 2018

@quinor Feel free to add a fixtures or testdata folder and add few YAML UAST files to it. Tests can read and diff those files. No need to run the whole client+bblfshd chain.

To get those YAMLs you can run the bblfsh-cli included in the latest Go client. It can connect to bblfshd, parse files and emit the YAML/binary formats.

@quinor
Copy link
Author

quinor commented Oct 30, 2018

Is there any documentation/examples for the go+CI we are using?

@dennwc
Copy link
Member

dennwc commented Oct 30, 2018

@quinor Don't think about CI right now, you can use go test ./... to test it locally. The same command will be executed by CI.

@quinor
Copy link
Author

quinor commented Oct 30, 2018

Did not realize there is an unified "go test" framework. That simplifies things alot :)

@quinor quinor force-pushed the master branch 2 times, most recently from b2681a8 to c65e770 Compare November 10, 2018 16:01
@@ -0,0 +1,36 @@
#!/usr/bin/env python3
Copy link
Member

Choose a reason for hiding this comment

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

The script is pretty simple. Can be written in Go as well (it's the only Python file here).

Copy link
Author

Choose a reason for hiding this comment

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

I only threw it in so that one may try to regenerate/generate new test cases. Also, see the answers below.

Copy link
Member

Choose a reason for hiding this comment

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

I still think it makes sense to rewrite it in Go. All the SDK is in Go. I see no reason to require Python installation for such a simple script.

@@ -0,0 +1,148 @@
720_1
Copy link
Member

Choose a reason for hiding this comment

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

Maybe there is a formal definition for this? For example "source size is less than N bytes/lines" or "the UAST size is less than N bytes/nodes"?

Copy link
Author

Choose a reason for hiding this comment

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

I'm not sure how I'd chosen these. If there was such definition, how to proceed?

Copy link
Member

Choose a reason for hiding this comment

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

The script can check the file size before writing it to the destination.

By the way, if the script was in Go, you can use the Babelfish client directly and count nodes / check the size of the source or the UAST.

Copy link
Author

Choose a reason for hiding this comment

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

The reason I wrote it in Python is I'm not really comfortable with file-processing scripts in Go. Python seemed like a better alternative to bash (that's what it would be written in otherwise).

Copy link
Author

Choose a reason for hiding this comment

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

Also, Python exists on every modern Linux distro so it's still quite portable (see the alternative: bash)

@quinor quinor force-pushed the master branch 2 times, most recently from 025d8bb to 8c1149e Compare December 12, 2018 10:32
@dennwc
Copy link
Member

dennwc commented Mar 6, 2019

Needs a rebase on top of a current master at least.

Also make sure to run go fmt ./...

Wojciech Jabłoński added 9 commits March 24, 2019 16:09
Signed-off-by: Wojciech Jabłoński <wj359634@students.mimuw.edu.pl>
Signed-off-by: Wojciech Jabłoński <wj359634@students.mimuw.edu.pl>
Signed-off-by: Wojciech Jabłoński <wj359634@students.mimuw.edu.pl>
Signed-off-by: Wojciech Jabłoński <wj359634@students.mimuw.edu.pl>
Signed-off-by: Wojciech Jabłoński <wj359634@students.mimuw.edu.pl>
Signed-off-by: Wojciech Jabłoński <wj359634@students.mimuw.edu.pl>
Signed-off-by: Wojciech Jabłoński <wj359634@students.mimuw.edu.pl>
Signed-off-by: Wojciech Jabłoński <wj359634@students.mimuw.edu.pl>
Signed-off-by: Wojciech Jabłoński <wj359634@students.mimuw.edu.pl>
Signed-off-by: Wojciech Jabłoński <wj359634@students.mimuw.edu.pl>
Wojciech Jabłoński added 2 commits March 24, 2019 16:51
Signed-off-by: Wojciech Jabłoński <wj359634@students.mimuw.edu.pl>
Signed-off-by: Wojciech Jabłoński <wj359634@students.mimuw.edu.pl>
@dennwc dennwc mentioned this pull request Apr 11, 2019
@dennwc
Copy link
Member

dennwc commented Apr 11, 2019

Have to close this PR - cannot rebase, because it's made from the master branch on the fork. See #396.

@dennwc dennwc closed this Apr 11, 2019
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.

3 participants