xflr6 / graphviz

Simple Python interface for Graphviz
https://graphviz.readthedocs.io
MIT License
1.63k stars 211 forks source link

Unexpected breaking change stripping newlines in body argument #157

Closed nyurik closed 2 years ago

nyurik commented 2 years ago

The change v0.17 -> 0.19.1 resulted in a breaking change that I couldn't find any documentation, and I'm not sure if it was intentional. A simple repo:

from graphviz import Digraph
g = Digraph('G', graph_attr=dict(rankdir='LR'), body=["str1", "str2", "str3"])
g.render("result", format="plain")

The resulting dot file produced with 0.17 (as expected):

digraph G {
    graph [rankdir=LR]
str1
str2
str3
}

The result with 0.19:

digraph G {
    graph [rankdir=LR]
str1str2str3}

After some debugging - 0.17 used "\n".join(body), whereas the new one uses for line in body: write(line) (via yield).

xflr6 commented 2 years ago

Thanks for the report.

The underlying change in 0.18 is documented in https://graphviz.readthedocs.io/en/stable/changelog.html#version-0-18 as follows:

Change of undocumented behaviour: When iterating over a Graph, Digraph, or Source instance, the yielded lines now include a final newline ('\n'). This mimics iteration over file object lines in text mode.

Also updated the relevant section on using .body directly (https://graphviz.readthedocs.io/en/stable/manual.html#custom-dot-statements) that the final newline needs to be included:

To add arbitrary statements to the created DOT source, you can use the body attribute of Graph and Digraph objects. It holds the verbatim list of (str) lines to be written to the source file (including their final newline).

Looks like we should also update the docstring of the body argument here:

https://github.com/xflr6/graphviz/blob/ee3ae102eb2c144ec36f0ca801ea590b85143ba4/graphviz/graphs.py#L99

xflr6 commented 2 years ago

Would this suffice? https://github.com/xflr6/graphviz/compare/body_newline

nyurik commented 2 years ago

@xflr6 would it make sense to just automatically append a newline if its missing? Documentation keeps saying "line", but if the line is not \n-terminated, it becomes concatenation -- an opposite of a "line" concept, which is almost never the desired result. I think to keep this as simple and surprise-free as possible, graphvis can just handle this edgecase, especially when it would be compatible with the prior versions. The fewer surprises we have with libs, the easier our life will be :)

xflr6 commented 2 years ago

SGTM assuming that we only change the behaviour or the body argument (not the attribute).

Let me know in case you want to give it a try.

xflr6 commented 2 years ago

Closing #158 in favour of a the documentation fix proposed earlier since the former also changes the .body attribute.

xflr6 commented 2 years ago

Pending fix in 77e5fcb703d02ba30c1cf81153fd0883c870c90b.

xflr6 commented 2 years ago

Fixed in 0.19.2 release, closing.