Split attribute quoting from ID quoting, rewrite attribute formatting, store names unquoted

[ferdnyc]

As the scope of #339 has creeped rather a lot and threatened to push it into a state of perpetual limbo, I've started splitting out some of the changes into more focused PRs. This is another one of those, though the rather far-ranging nature might make it seem otherwise. This PR affects:

Attribute formatting

• Separate ID and attribute quoting, the same way #339 started out doing
• Fix quoting for attribute values
• Completely rewrite the attribute formatting process, moving the meat of the work into new, centralized Common methods, which all classes' to_string() methods now call to get their formatted attributes
• Remove the attribute sorting, stomping on @lkk7's #361 (sorry! I did cherry-pick your test fixes into this PR, though.)

Name storage & formatting

• Remove all quoting of ID strings on input, switching to instead always performing quoting on output, so that strings are stored internally the same way the user inputs them, making lookups via .get_*() methods less confusing/annoying.
• Take advantage of the fact that we know when we're outputting a name vs. some other syntax, and quote graphviz keywords used as names to avoid syntax errors in the generated source. Make exception for the default-values node names.

Closes #361 Closes #355 Closes #181 Fixes #358 Fixes #282 Fixes #258 Fixes #246 Fixes #180 Fixes #111 Fixes #85

[ferdnyc]

(Actually I think #85 was already fixed by #320, but it never got closed so might as well close it here.)

[github-actions[bot]]

This pull request has conflicts, please resolve those so that the changes can be evaluated.

[github-actions[bot]]

All conflicts have been resolved, thanks!

[ferdnyc]

The tests probably do cover all of this, but I'd feel better adding some extra tests specifically for the rewritten functionality.

I started writing some, then stumbled on the fact that it isn't always easy to compare values in the test suite. (On the plus side, trying to compare Node values was how I stumbled on the broken parenting behavior that led to #365.)

[ferdnyc]

@lkk7

OK, I have just now added sufficient new tests targeted specifically at the functionality in this PR, that I think I can declare it working and verified. (I also fixed the items brought up in review.)

I hit on a method of testing that the get_{node,edge,etc.}() methods were returning the expected objects (even though they won't be the same Python objects, because they're recreated each time a value is returned) by comparing the id() value of the obj_dict inside each object.

If, for example, Graph.get_node() returns a particular Node object, it won't be the same Node object that was originally passed to Graph.add_node(), but it will contain the same obj_dict that was originally passed in, because those are all stored by reference and don't get copied internally. So, code like this in the tests works, where direct comparison of the objects would not:

g = pydot.Graph()
a = pydot.Node("my node")
g.add_node(a)
a_out = g.get_node("my node")[0]
self.assertEqual(id(a_out.obj_dict), id(a.obj_dict))

I may even define __eq__ dunder methods for Node, Edge, etc. that implement that check, so that we can use some_node == some_returned_node or self.assertEqual(some_node, some_returned_node) and get the correct results. (It'd make the tests I just wrote a lot cleaner, for one.) But that can be a followup PR.

[ferdnyc]

If I'm not mistaken all I've done there is a poor-man's reimplementation of is, since a is b is effectively a concise way of saying id(a) == id(b). unittest does have an assertIs(), so the test could theoretically also be written:

g = pydot.Graph()
a = pydot.Node("my node")
g.add_node(a)
a_out = g.get_node("my node")[0]
self.assertIs(a_out.obj_dict, a.obj_dict)

[tusharsadhwani]

Amazing work!

[lkk7]

@ferdnyc Well, I shouldn't close the issues until the fixes are properly released. I think now is a good time for a release, so I'll prepare a changelog

[ferdnyc]

@lkk7

Oh... Oops. Sorry, they already closed. The PR was set to auto-close all of them.

I shouldn't close the issues until the fixes are properly released.

I mean, I guess there are two schools of thought on that. As soon as the fix is in the repo, the issue is solved. Anyone who wants to run with the master branch can always install the development code with:

pip install git+https://github.com/pydot/pydot.git

(Hmmm... we should probably add that to the README.)

[ferdnyc]

(Ugh, that reminds me. As much of a pain in the ass as it can be, it's probably a mitzvah if we take the time to rename the master branch to main.)

[lkk7]

Oh, that's fine. The release will be coming soon anyway.

I'll add that note to README.md and I'll change the branch name. Edit: i see you're the README update. 😄 But I'll do the branch rename