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

String attributes containing colon are not quoted #258

Open
stefanv opened this issue Mar 8, 2021 · 9 comments · May be fixed by #339
Open

String attributes containing colon are not quoted #258

stefanv opened this issue Mar 8, 2021 · 9 comments · May be fixed by #339

Comments

@stefanv
Copy link

stefanv commented Mar 8, 2021

import pydot
n = pydot.Node('x', fillcolor='red:yellow')
print(n)

Seen:

x [fillcolor=red:yellow];

Expected:

x [fillcolor="red:yellow"];

(dot won't compile without the value being quoted, complaining of Error: file.dot: syntax error in line x near ':')

This behavior has been confirmed on pydot 2.0.0.dev0.

@stefanv
Copy link
Author

stefanv commented Mar 8, 2021

This was originally reported as networkx/networkx#4663

@peternowee
Copy link
Member

Thanks for reporting. This is probably very related to #118 and #224. Even though those issues are about colons in node/edge names and you are reporting about a colon in a DOT attribute value, both are DOT IDs. Not sure yet if this is a complete duplicate, though, so I suggest we keep this issue open so we can include it as another test case.

All these issues should get fixed as part of the next pydot release (2.0.0) in my opinion, though be prepared that it will still be a while before that gets released. Yesterday @MaxHorwood said in #224 he would try to work on a PR.

@stefanv
Copy link
Author

stefanv commented Mar 8, 2021

That's great; thanks for the update @peternowee!

@horvatha
Copy link

There is a workaround. One can use fillcolor='"red:yellow"' (double quote inside the string). But I wouldn't assume that many pydot-user will find it out easily.

@julianfortune
Copy link

@peternowee Hope you are doing well 😄 I am still interested in finding a solution for this and it looks like you are too!

Two proposals have occurred to me, and I hope you will indulge me:

  1. Change the README so that this line:

    my_node = pydot.Node('a', label='Foo')

    becomes:

    my_node = pydot.Node('a', label='"Foo"')

    and add a little explanation about string literals, so first-time users know how to avoid the problem.

  2. Or a massive refactor to use types to differentiate string literals directly:
    As an example, if something like this is added to the library:

    StringLiteral = namedtuple('StringLiteral', ['value'])  # May want to use dataclass, but this is shorter
    Port = namedtuple('Port', ['something', 'anotherthing'])  # Still don't understand this, sorry
    ...
    
    Label = Union[StringLiteral, Port, ... etc]  # Don't know what the options are, apologies again

    Then the user must be explicit about what type of label they would like, and it could be error handled:

    my_node = pydot.Node('a', label=StringLiteral('Foo'))

    This would probably involve a lot of work and would make PyDot less of a wrapper, but would enable better error handling and messaging to the user.

I am curious if you or other contributors have proposals for how to address this.

@laowantong
Copy link

Unfortunately, the workaround function:

def _check_colon_quotes(s):
    # A quick helper function to check if a string has a colon in it
    # and if it is quoted properly with double quotes.
    # refer https://github.com/pydot/pydot/issues/258
    return ":" in s and (s[0] != '"' or s[-1] != '"')

... fails when s is not a string (e.g., a numeric weight).

@horvatha
Copy link

horvatha commented Oct 13, 2022

@laowantong I can't follow you, where is this workaround function? I can't find in the source code. It's your own? Why not just

def _check_colon_quotes(s):
    # A quick helper function to check if a string has a colon in it
    # and if it is quoted properly with double quotes.
    # refer https://github.com/pydot/pydot/issues/258
    if isinstance(s, str):
        return ":" in s and (s[0] != '"' or s[-1] != '"')
    else:
        return False

or you can use s = str(s) before the return statement.

@laowantong
Copy link

@horvatha Sorry, I should have made myself clearer. This is not my code, but a function of NetworkX:

https://github.com/networkx/networkx/blob/ce692bd3f05900608b829b983838d099b378ca8f/networkx/drawing/nx_pydot.py#L195-L199

I am not sure how it is connected to this project though. My own workaround was to convert to string all the weights of my graph to be able to save it:

    def save(self, stem_or_path):
        # workaround for https://github.com/pydot/pydot/issues/258#issuecomment-1276492779.
        # Convert all edges attributes to strings.
        # The same should be done for nodes attributes, but YAGNI.
        G = deepcopy(self)
        for (x, y, data) in G.edges(data=True):
            for (k, v) in list(data.items()):
                G[x][y][k] = str(v)
        smart_path(stem_or_path).write_text(nx.nx_pydot.to_pydot(G).to_string())

@ferdnyc
Copy link
Contributor

ferdnyc commented Apr 30, 2024

Honestly, IMHO the right fix is to simply ensure that values are quoted by pydot whenever they need to be. I see this still isn't fixed, looks like quote_if_necessary needs another upgrade.

@ferdnyc ferdnyc linked a pull request Apr 30, 2024 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants