Extensions #28
Extensions #28
Conversation
There was a problem hiding this comment.
This PR appears to have two unrelated purposes:
- some kind of fix for
self._exts - a new --attrs feature
Both have some minor problems (wrong .so extension, missing semicolon for attrs).
I would prefer two separate PRs for each. Also, I would like to have a PR description that's suitable for pasting into the changelog.
| digraph ModuleDependencies { | ||
| rankdir=TB |
There was a problem hiding this comment.
Shouldn't there be a ; at the end of this line?
There was a problem hiding this comment.
As per documentation attributes has no semicolon
There was a problem hiding this comment.
But the grammar at https://graphviz.org/doc/info/lang.html shows that a graph has a ;-separated statement list inside the { }, and ID = ID is just one possible form of a statement.
I admit that I only checked the grammar and haven't actually tried running dot on any of the example graphs.
There was a problem hiding this comment.
I have tried it now, and dot from graphviz 2.43.0 accepts both spellings (with ; and without ;), and then rewrites them both into graph [rankdir=TB];
Oh! I misread the grammar! The ; separator in the statement list is optional everywhere!
Still, since findimports adds ; after nodes and edges, I'd like it to add ; after attributes as well, for consistency.
| self._exts.append(f".{sys._multiarch}.so") | ||
| if hasattr(sys, 'platform'): | ||
| ver = ''.join(map(str, sys.version_info[:2])) | ||
| self._exts.append(f'.cpython-{ver}-{sys.platform}.so') |
There was a problem hiding this comment.
I don't think this is right. sys.platform is "linux", but all the modules in /usr/lib/python3.12/lib-dynload/ use "x86_64-linux-gnu" in this spot on my machine.
You want sysconfig.get_config_var('EXT_SUFFIX') here.
(Personally, I don't even remember what self._exts is used for. Discovering whether named Python modules exist on disk?)
There was a problem hiding this comment.
It is a fallback for determining if it is a module.
| @@ -51,6 +51,8 @@ | |||
| nothing are removed. | |||
| -D MAX_DEPTH, --depth MAX_DEPTH | |||
| import depth in ast tree. Default: no limit | |||
| --attrs ATTRIBUTES Add custom attributes to dot graph. | |||
| Specified as json string. | |||
There was a problem hiding this comment.
Wouldn't it be more convenient to do -a rank=TB instead of --attrs '{"rank": "TB"}'? Spelling out JSON on the command line is a pain.
There was a problem hiding this comment.
Hm, depends on how many attributes. If it is just one or 2 I go with multiple-argument flag, if it can be more of a small config size, I use JSON and quite happy about it. But I don't see this used often so I am ok with your suggestion as well.
|
Split into 2 other PRs |
a) Correct extending
_extb) Support for
macspecific extension modulesc) Support custom attributes for
dotgraph