Skip to content
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

Vertical line labels #33

Merged
merged 5 commits into from
Oct 2, 2018
Merged

Vertical line labels #33

merged 5 commits into from
Oct 2, 2018

Conversation

etimberg
Copy link
Contributor

Adds support for labeling vertical lines. This was easier said than done. I think it might help overall if it was easier to look for a character at a given grid square (perhaps an accessor around the string?). That way this could all be done in one pass rather than 2 but I think that might be better in it's own PR.

From the test:

def test_vertical_line_labels():
    graph = graph_from_ascii("""
        A
        |
      (Vertical)
        |
        B
    """)

@coveralls
Copy link

coveralls commented Sep 30, 2018

Coverage Status

Coverage increased (+0.6%) to 97.368% when pulling 6fe199b on vertical-line-labels into 83a1301 on master.

@greatestape
Copy link
Contributor

What should this do? Error?

        A     C
        |     |
      (Vertical)
        |     |
        B     D

@greatestape
Copy link
Contributor

Similar case

        A     
        |     
      (Vertical)--C
        |     
        B     

@etimberg
Copy link
Contributor Author

etimberg commented Oct 1, 2018

I think both those cases are errors because a wire can only connect two nodes and both cases violate that

@AnjoMan thoughts?

@AnjoMan
Copy link
Member

AnjoMan commented Oct 2, 2018

I agree those are both error cases. (label) is not a node

@AnjoMan AnjoMan merged commit 596c8cd into master Oct 2, 2018
@etimberg etimberg deleted the vertical-line-labels branch October 2, 2018 14:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants