[WIP] Support for graphviz dot #109

lgalke · 2017-05-05T23:10:09Z

Hey @AndrewRadev
This is my initial version for graphviz dot support. While it works as intended for nodes that are variables (in most cases this holds), it lacks robustness against string node names which contain special characters. For instance, I started with split(line, ';') instead of using proper regular expressions. Thus a node identifier like"this, makes;-> problems" is not covered yet. However, what you typically do in dot is declaring nodes as variables where you can add any label once, instead of using it as the node identifier directly (which you would always have to repeat in edge definitions) e.g.:

X [label="some super complex label, and stuff; -> something"]
X -> A, B;

Splitjoin objects

Multiple statements (A -> B; X -> Y)
Multi-edges (A -> B,C)
Chained (transitive) edges (A -> B;\n B->C)

On splitting callbacks are issued top-down. On joining, bottom-up.
A possible improvement for the second and third case would be to also consider joining two statements on a single line (for now it only works if there is 1 statement per line in two consecutive lines).
Now, I included the case of joining two statements on a single line. Still, the behavior could be even more sensitive to cursor position (e.g. join statement 2 and 3 on one line if the cursor is in statement 2).

Two examples:

// Example 1
// Cursor on 'A'

A, B -> C -> D -> E; X -> Y;

// gS

A, B -> C -> D -> E;
X -> Y;

// gS

A, B -> C;
C -> D;
D -> E;
X -> Y;

// gS

A -> C;
B -> C;
C -> D;
D -> E;
X -> Y;

// gJ

A, B -> C;
C -> D;
D -> E;
X -> Y;

// gJ

A, B -> C -> D;
D -> E;
X -> Y;

// gJ

A, B -> C -> D -> E;
X -> Y;

// gJ

A, B -> C -> D -> E; X -> Y;


// Example 2
// Cursor on 'A'

A, B -> C, D;

// gS

A -> C;
A -> D;
B -> C;
B -> D;

// gJ

A -> C, D;
B -> C;
B -> D;

// jgJ

A -> C, D;
B -> C, D;

// kgJ

A, B -> C, D;

lgalke · 2017-05-05T23:33:27Z

#105

lgalke · 2017-05-05T23:59:00Z

Why do checks fail after just dropping a single echo statement, whereas previous commits succeeded? Strange.

AndrewRadev · 2017-05-07T09:33:42Z

The tests seem to pass after rerunning the build. I saw something about some rust tests in the failure, so maybe there was an issue with TravisCI (I disabled the rust tests on travis, long story).

Anyway, the existing are fine, but I guess that's to be expected, it's support for a brand new thing. I'm going to write some tests for the new code soon, and I'll push them to this PR (since recently, github allows me to do that :)).

I've left some comments. In general, it looks great, and I really appreciate your work. You've definitely thought of a lot of cases :). It'll be easy to build a good test suite for the code.

We might have to restructure it, though. I pointed it out in one comment, that taking the line and working with just the string is usually not enough. More often than not, parsing the contents of the line requires working with the text in the buffer as much as possible, since the buffer text contains syntax information. I definitely think it's doable, though.

I'll see what I can do about the tests, I'm a bit busy these days, but I'll get more free time in a couple of weeks. If you run into issues, and/or are not sure how exactly to implement something, feel absolutely free to write a comment in the PR, ask me for advice, I might even implement it myself, if I can allocate the time.

lgalke · 2017-05-08T13:41:07Z

Thanks. Where exactly can I find the comments you left in the code?

AndrewRadev · 2017-05-08T13:43:24Z

You can see them in this pull request. If you click on "files changed", you'll end up on this page: https://github.com/AndrewRadev/splitjoin.vim/pull/109/files. Scroll, and you'll see my comments inline in the diff.

lgalke · 2017-05-12T07:28:40Z

Hmm, I still cannot access the review comments 😢

AndrewRadev · 2017-05-07T09:13:20Z

autoload/sj/dot.vim

+let s:node = '\("*[^\"]\{-}"\|\i\+\)'
+
+" Helper functions {{{
+function! sj#dot#ExtractNodes(side)


In general, I like to keep the helper functions at the bottom of the file, after the public interface. I also prefer to keep them script-local (starting with s:) most of the time. I only use sj#whatever#Func for functions that should be shared across different files. I think this one (and a few more) can be script-local.

AndrewRadev · 2017-05-07T09:18:18Z

autoload/sj/dot.vim

+  " INPUT: 'A, B -> C -> D'
+  " OUTPUT: [[[A, B], [C]], [[C], [D]]]
+  let statement = s:TrimSemicolon(a:statement)
+  " FIXME will fail if '->' inside "s


There's two ways we can avoid this issue. One is to check the syntax under the cursor. The syntax group under the arrow, for me at least, is dotKeyChar. You can use the sj#SearchSkip function to search for arrows that only fit this syntax pattern. Grep through the codebase for usage examples.

Alternatively, you could use an argparser. The json one is the most common one I use, and I think you can just replace the check for a comma with a check for ->. It works by "inheriting" a "class" with some common code: https://github.com/AndrewRadev/splitjoin.vim/blob/6095f461651c2416cc31b52039806b9e52428388/autoload/sj/argparser/common.vim

You can probably use the parser for the comma-separated entries for the nodes as well. I'll comment on the other fixme.

AndrewRadev · 2017-05-07T09:19:51Z

autoload/sj/dot.vim

+  " Try to eat the next line
+
+  call sj#PushCursor()
+  " FIXME Dangerous on EOF?


You can check for the end of file by consulting line('$'). Probably, if line('.') + 1 is equal to line('$'), you can't really do anything and you might as well return a "failure" (whatever constitutes a failed result here).

AndrewRadev · 2017-05-07T09:20:51Z

autoload/sj/dot.vim

+
+function! s:Edge2string(edge)
+  " INPUT: [[src_nodes], [dst_nodes]]
+  " OUTPUT: string representation of the aequivalent statement


A very minor thing, but I think I'd prefer these on top of the function definition. Honestly, there's no practical reason for it other than consistency with existing code. I do like the comments themselves, though, quite useful.

49beb62 addressed this.

AndrewRadev · 2017-05-07T09:27:24Z

autoload/sj/dot.vim

+  " Split multiple nodes into single elements
+  " INPUT: 'A, B, C'
+  " OUTPUT: ['A', 'B', 'C']
+  " FIXME will fail on 'A, B, "some,label"'


You can use an argparser for this. This JSON one might work out of the box: https://github.com/AndrewRadev/splitjoin.vim/blob/6095f461651c2416cc31b52039806b9e52428388/autoload/sj/argparser/js.vim.

The challenge would be restructuring your code so you don't provide a string, but an area of the buffer. For more complicated processing, sadly, just taking the string doesn't work out well in practice -- in the buffer, you can move the cursor, and you can check syntax items under it.

AndrewRadev · 2017-05-12T07:38:22Z

Ugh, I guess it was my fault, sorry. Turns out, the comments were somehow batched, and I needed to "Submit review" to make them visible. I really didn't expect this. Might be some recent change in how github comments work.

Anyway, sorry about this, but they should be visible now.

AndrewRadev · 2017-07-02T07:24:54Z

I've added some test to the dot support, in order to exercise it. I've found one issue that I fixed, but I'm not sure about the other one. Basically, joining this doesn't seem to work correctly:

A, B -> C -> D;
D -> E;

The result seems to be:

A, B -> C -> D; D -> E;

The tests are in this commit, because I couldn't push them to the PR: 8f61383

…ub.com/AndrewRadev/splitjoin.vim

lgalke · 2017-07-03T08:31:31Z

Thanks. I merged the commit with the test cases and will continue working on the PR. 👍

lgalke added 13 commits May 3, 2017 01:31

dot support for splitting edges

5c4112e

complete matching function start

94a7868

support for multi-edges

8684da1

bugfix

d357192

robust splitting also on multi-statements

729cfe4

joining close to working, two more bugs to fix

042c189

two different use cases

a7d979c

release candidate

a6aef8f

some more robustness

30a3250

todo statement

5fdcc51

release candidate 1

b8490bc

rc2

8bacdd0

documentation

4e03adb

lgalke mentioned this pull request May 5, 2017

Add support for graphviz #105

Open

lgalke added 2 commits May 6, 2017 01:49

support for 2 statements on 1 line

1b9582d

dropped echo statements

6095f46

AndrewRadev requested changes May 12, 2017

View reviewed changes

lgalke and others added 6 commits May 13, 2017 20:11

script-local helper functions

6fd6a28

EOF safe

2b9adf6

a very minor thing

49beb62

buffer work starting point

7728b66

Merge branch 'master' of https://github.com/AndrewRadev/splitjoin.vim

712af9f

Specs for dot support (failing for now)

8f61383

lgalke added 2 commits July 3, 2017 04:28

Merge branch 'master' of git://github.com/AndrewRadev/splitjoin.vim

b207a01

Merge commit '8f61383a20e491b496aadfd52921e6f8139e4e29' of git://gith…

0534e34

…ub.com/AndrewRadev/splitjoin.vim

Base automatically changed from master to main January 29, 2021 10:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Support for graphviz dot #109

[WIP] Support for graphviz dot #109

lgalke commented May 5, 2017 •

edited

Loading

lgalke commented May 5, 2017

lgalke commented May 5, 2017

AndrewRadev commented May 7, 2017

lgalke commented May 8, 2017

AndrewRadev commented May 8, 2017

lgalke commented May 12, 2017

AndrewRadev May 7, 2017

lgalke May 13, 2017

AndrewRadev May 7, 2017

AndrewRadev May 7, 2017

lgalke May 13, 2017

AndrewRadev May 7, 2017

lgalke May 13, 2017

AndrewRadev May 7, 2017

AndrewRadev commented May 12, 2017

AndrewRadev commented Jul 2, 2017

lgalke commented Jul 3, 2017

[WIP] Support for graphviz dot #109

Are you sure you want to change the base?

[WIP] Support for graphviz dot #109

Conversation

lgalke commented May 5, 2017 • edited Loading

Splitjoin objects

Two examples:

lgalke commented May 5, 2017

lgalke commented May 5, 2017

AndrewRadev commented May 7, 2017

lgalke commented May 8, 2017

AndrewRadev commented May 8, 2017

lgalke commented May 12, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndrewRadev commented May 12, 2017

AndrewRadev commented Jul 2, 2017

lgalke commented Jul 3, 2017

lgalke commented May 5, 2017 •

edited

Loading