Tighten up our API with keyword-only arguments #492
Conversation
Instead of `Any`, only accept `str | int | float`.
To tighten up the way the API is used, add an `*` to the argument
list of appropriate methods, to signify that the arguments beyond
that point are keyword-only.
Specifically:
- in `to_string` methods, `indent` is keyword-only (as is the
internal `indent_level`)
- in `del_` methods, the optional `index` is keyword-only
- `quote_id_if_necessary`'s `unquoted_keywords` is keyword-only
- in all constructors except `Common.__init__`, `obj_dict` is
keyword-only, and moved to the end of the keyword argument list.
(It's not kw-only in `Common`, because it's the ONLY argument and
the other constructors call it as `super().__init__(obj_dict)`.)
- Constructor arguments like `strict`, `suppress_disconnected`, etc.
for `Graph` types are keyword-only.
- `graph_type` is explicitly _not_ kw-only, meaning a graph
could be constructed as `pydot.Graph("G", "digraph")` which seems
perfectly fine.
- `Dot.create` used to be called internally by `Dot.write` as
`Dot.create(prog, format, encoding=)` which didn't seem like a
very comfortable API for outside users, so now the `create` method
takes `format` as the only positional argument, and the call is
changed to `Dot.create(format, prog=, encoding=)`
- `Dot.write` itself takes only the `path` as a positional argument,
the rest are keyword-only. While it might be nice to allow something
like `Dot.write('/some/path', 'pdf')`, since the format is the
_first_ positional argument of `Dot.create()` that's potentially
confusing. (Ideally maybe `Dot.write()` should be able to infer the
format based on the filename.)
Coverage report
Click to see where and how coverage changed
This report was generated by python-coverage-comment-action |
||||||||||||||||||||||||
|
This is technically an API change. In fact, it's a fairly significant one if you happen to be abusing the API with things like I think it's still a good idea, but I guess it should be targeted for Pydot 5.0. |
|
I really am wondering if I should make d.write('/tmp/mygraph.svg') # (format="svg")
d.write('/tmp/mygraph.png') # (format="png")
d.write('/tmp/mygraph.jpg') # (format="jpg")
We might want a couple of special cases (like (Basically, I just really hate that our default format is |
There was a problem hiding this comment.
This will probably break a lot of old code, but it's a cool change 😄 Definitely for 5.0! (BTW, 5.0? It flew by so quickly)
Maybe I'll scan some more well-known repos, like networkX, in my free time to see how this would affect them. Although good luck finding them there: https://github.com/pydot/pydot/network/dependents
|
Ideally I'd get un-lazy and add some tests before this gets merged, anyway. So, no rush. |
|
In addition to the tests I've so far still been too lazy to write, I wonder if there's enough old, breakable code out there to make it worth providing a transitional wrapper layer for this? Without too much work, I could add a module # src/pydot/legacy.py
import pydot.core
class Node(pydot.core.Node):
def __init__(
self,
name: str = "",
obj_dict: AttributeDict | None = None,
**attrs: Any,
) -> None:
super().__init__(name, obj_dict=obj_dict, **attrs)
class Graph(pydot.core.Graph):
def __init__(
self,
graph_name: str = "G",
obj_dict: AttributeDict | None = None,
graph_type: str = "digraph",
strict: bool = False,
suppress_disconnected: bool = False,
simplify: bool = False,
**attrs: Any,
) -> None:
super().__init__(
graph_name,
graph_type,
obj_dict=obj_dict,
strict=strict,
suppress_disconnected=suppress_disconnected,
simplify=simplify,
**attrs)
def del_node(self, name: str | Node, index: int | None = None) -> bool:
return super().del_node(name, index=index)
# etc...Might need some Then, someone upgrading who has calls that violate the new API has the choice between either fixing their calls, or changing their imports to pull in the - from pydot import Graph, Node, Edge
+ from pydot.legacy import Graph, Node, EdgeOr, if I also import all of the unchanged symbols into - import pydot
+ import pydot.legacy as pydotObviously, ...Ooh, one potential wrinkle: All of the methods that create objects may also have to be overridden, since (for example) def get_node_list(self) -> list[Node]:
node_objs: list[Node] = []
for node in self.obj_dict["nodes"]:
obj_dict_list = self.obj_dict["nodes"][node]
node_objs.extend([Node(obj_dict=obj_d) for obj_d in obj_dict_list])
return node_objs...and for (This is probably a sign that e.g. Edit: Actually, not even |
|
I'd like to answer at length, but I agree with everything, so... - import pydot
+ import pydot.legacy as pydotSo it's fine by me. |
If it works. I still have to puzzle through all of the implications of that, beyond the Graph class's uses of other class constructors. Offhand, the entirety of |
|
Wait... I'm an idiot. Nothing about def graph_from_dot_data(s: str) -> list[pydot.legacy.Dot] | None:
graphs = pydot.core.graph_from_dot_data(s)
if graphs is not None:
return [
pydot.legacy.Dot(obj_dict=g.obj_dict)
for g in graphs
]
return graphsDone. Same for every other Maybe a generator could use some Eh. I guess there's only six |
To tighten up the way the API is used, add an
*to the argumentlist of appropriate methods, to signify that the arguments beyond
that point are keyword-only.
Specifically...
in
to_stringmethods,indentis keyword-only (as is theinternal
indent_level)in
del_methods, the optionalindexis keyword-onlyquote_id_if_necessary'sunquoted_keywordsis keyword-onlyin all constructors except
Common.__init__,obj_dictiskeyword-only, and moved to the end of the keyword argument list.
(It's not kw-only in
Common, because it's the ONLY argument andthe other constructors call it as
super().__init__(obj_dict).)Constructor arguments like
strict,suppress_disconnected, etc.for
Graphtypes are keyword-only.graph_typeis explicitly not kw-only, meaning a graphcould be constructed as
pydot.Graph("G", "digraph")which seemsperfectly fine.
Dot.createused to be called internally byDot.writeasDot.create(prog, format, encoding=)which didn't seem like avery comfortable API for outside users, so now the
createmethodtakes
formatas the only positional argument, and the call ischanged to
Dot.create(format, prog=, encoding=)Dot.writeitself takes only thepathas a positional argument,the rest are keyword-only. While it might be nice to allow something
like
Dot.write('/some/path', 'pdf'), since the format is thefirst positional argument of
Dot.create()that's potentiallyconfusing. (Ideally maybe
Dot.write()should be able to infer theformat based on the filename.)
Rationale
It used to be possible, at least in theory, to make calls like these:
...all of which seem inadvisable and best avoided.
We're pretty good about keyword args in our own code. (I only had to change one call to conform with this, an ancient
create()call inDot.writethat used a weird set of positional arguments.) But, still, better that it's enforced by the method definitions. So, create some enforcement.With this change, it's made explicit that the correct way to make the calls above is:
Other changes
A second commit tightens the type for
.to_string(indent)fromAnytostr | int | float.