You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@buildstream.apache.org by "harrysarson (via GitHub)" <gi...@apache.org> on 2025/01/07 14:17:15 UTC
[PR] Escape names when using as node_id [buildstream]
harrysarson opened a new pull request, #1979:
URL: https://github.com/apache/buildstream/pull/1979
Colons in node_ids generate invalid graphviz, therefore escape the name
and use a `label` attribute to rendor correct name in the graph.
This PR also uses a raw string for the regex to fix `SyntaxWarning: invalid escape sequence '\|'`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@buildstream.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] contrib/bst-graph: Escape names when using as node_id [buildstream]
Posted by "harrysarson (via GitHub)" <gi...@apache.org>.
harrysarson commented on code in PR #1979:
URL: https://github.com/apache/buildstream/pull/1979#discussion_r1944769051
##########
contrib/bst-graph:
##########
@@ -55,6 +56,10 @@ def parse_args():
return parser.parse_args()
+def escape(s):
+ return urllib.parse.quote_plus(s.encode())
Review Comment:
> Can you please add a small comment that URI percent-encoding is used to generate unique identifiers that don't contain colons or backslashes?
Added a small comment, also renamed the `escape` function to `unique_node_name`. Hopefully that makes the intent easier to grok for readers.
> Also, is the .encode() part actually necessary? urllib.parse.quote_plus() accepts strings and the default encoding is utf-8, so it seems to me that .encode() is redundant, or am I missing something?
Fixed
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@buildstream.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] contrib/bst-graph: Escape names when using as node_id [buildstream]
Posted by "harrysarson (via GitHub)" <gi...@apache.org>.
harrysarson commented on PR #1979:
URL: https://github.com/apache/buildstream/pull/1979#issuecomment-2639897925
> Please prefix your commit summaries with `contrib/bst-graph: `
Done
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@buildstream.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Escape names when using as node_id [buildstream]
Posted by "juergbi (via GitHub)" <gi...@apache.org>.
juergbi commented on PR #1979:
URL: https://github.com/apache/buildstream/pull/1979#issuecomment-2598686539
Please prefix your commit summaries with `contrib/bst-graph: `
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@buildstream.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Escape names when using as node_id [buildstream]
Posted by "juergbi (via GitHub)" <gi...@apache.org>.
juergbi commented on code in PR #1979:
URL: https://github.com/apache/buildstream/pull/1979#discussion_r1920408451
##########
contrib/bst-graph:
##########
@@ -55,6 +56,10 @@ def parse_args():
return parser.parse_args()
+def escape(s):
+ return urllib.parse.quote_plus(s.encode())
Review Comment:
I see. What confused me is that graphviz doesn't actually expect URI/percent-encoded strings. As far as I can tell, this is using URI percent-encoding simply as a convenient way to generate unique identifiers that don't contain colons or backslashes. As labels are used, the identifiers don't really matter. (A cryptographic hash could be used instead but that would make the graphviz file less readable).
Can you please add a small comment that URI percent-encoding is used to generate unique identifiers that don't contain colons or backslashes?
Also, is the `.encode()` part actually necessary? `urllib.parse.quote_plus()` accepts strings and the default encoding is `utf-8`, so it seems to me that `.encode()` is redundant, or am I missing something?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@buildstream.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] contrib/bst-graph: Escape names when using as node_id [buildstream]
Posted by "juergbi (via GitHub)" <gi...@apache.org>.
juergbi merged PR #1979:
URL: https://github.com/apache/buildstream/pull/1979
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@buildstream.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Escape names when using as node_id [buildstream]
Posted by "harrysarson (via GitHub)" <gi...@apache.org>.
harrysarson commented on code in PR #1979:
URL: https://github.com/apache/buildstream/pull/1979#discussion_r1912918708
##########
contrib/bst-graph:
##########
@@ -55,6 +56,10 @@ def parse_args():
return parser.parse_args()
+def escape(s):
+ return urllib.parse.quote_plus(s.encode())
Review Comment:
`graphviz.escape()` does not escape colons:
See also the note here on colons https://graphviz.readthedocs.io/en/stable/manual.html#node-ports-compass
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@buildstream.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Escape names when using as node_id [buildstream]
Posted by "juergbi (via GitHub)" <gi...@apache.org>.
juergbi commented on code in PR #1979:
URL: https://github.com/apache/buildstream/pull/1979#discussion_r1910505690
##########
contrib/bst-graph:
##########
@@ -55,6 +56,10 @@ def parse_args():
return parser.parse_args()
+def escape(s):
+ return urllib.parse.quote_plus(s.encode())
Review Comment:
Is this the right function to use for escaping? Might [graphviz.escape()](https://graphviz.readthedocs.io/en/stable/api.html#graphviz.escape) be a better fit (I haven't tested it)?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@buildstream.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org