-
Notifications
You must be signed in to change notification settings - Fork 145
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Potential fix to the tooltip with special characters bug #1203
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you push the correct branch in your PR? The code change here is adding a print statement (it looks like a debug print) and not fixing the bug.
ohh sorry Let me check whats happening, |
@mtreinish Sorry about the print statement , is this how the output must look like ? it would be saved as a graph.svg ?? I am sorry looks like i forgot to push the update , Silly |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the idea is good, but we definetely need to check if we covered all possible characters that need escaping. And add tests
I belive using a package like serde json should be a better way to do it that building a dictionary of cases that we cant escaped |
Yes We can add a custom test case that tests for combinations of escape sequences |
tests/visualization/test_graphviz.py
Outdated
return {"label": label, "tooltip": tooltip} | ||
|
||
# Draw the graph using graphviz_draw | ||
image = graphviz_draw( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be better to test with https://www.rustworkx.org/dev/apiref/rustworkx.PyGraph.to_dot.html
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sounds good will work on em
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@IvanIsCoding Do you want me to run the code on the site or ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I want you to avoid writing temporary files to the disk for the unit tests. For the GitHub CI, we found those tests very flaky on macOS. So I suggested that method as a way of testing the fix with only in-memory operations
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good, Update the code with the recommendation, let me know if it needs any changes
Rust code is looking good, just two minor things and this will be good to go:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@anushkrishnav I approved the CL but you need to fix the CI errors by running black
, cargo clippy
. I also recommend running nox -edocs
to confirm that the documentation compiles too
Will finish it today, thanks it was fun to work with !! |
Pull Request Test Coverage Report for Build 9449570809Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
That is an unrelated error. I think the real one is highlighted in the CI:
The release note drawing is throwing an exception when calling graphviz |
Okay I see the problem is with the quotes , it helps with the '' issue, once i changed it back its back to normal, I will updated the expected to make it in line with using quotes. Can you try again ? @IvanIsCoding |
Pull Request Test Coverage Report for Build 9466635439Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Leaving some minor change requests as unwrap()
can panic the Rust code which is not very nice to handle from Python. If we return a result, though, it is much simpler
releasenotes/notes/fix-graphviz-draw-tooltip-3f697d71c4b79e60.yaml
Outdated
Show resolved
Hide resolved
releasenotes/notes/fix-graphviz-draw-tooltip-3f697d71c4b79e60.yaml
Outdated
Show resolved
Hide resolved
releasenotes/notes/fix-graphviz-draw-tooltip-3f697d71c4b79e60.yaml
Outdated
Show resolved
Hide resolved
Pull Request Test Coverage Report for Build 9473598112Details
💛 - Coveralls |
releasenotes/notes/fix-graphviz-draw-tooltip-3f697d71c4b79e60.yaml
Outdated
Show resolved
Hide resolved
Pull Request Test Coverage Report for Build 9473770325Details
💛 - Coveralls |
Awesome, thank you @IvanIsCoding I was fun working on this, Will work on more issue , I am learning rust for a the past 2 months and been exploring the code base to put my learning into practice thanks for the oppurtunity |
This commit fixes a regression introduced in Qiskit#1203. That PR was attempting to fix missing character escaping in some string fields but it was doing so too eagerly. This would result in invalid dot files being produced for users upgrading from rustworkx < 0.15.0 that were wrapping strings in quotes as needed previously. For example if you were setting `'color="#aaaaaa"'` previously before this would become `color="\"#aaaaaa\""` after Qiskit#1203. In order to quickly release a 0.15.1 this commit reverts the dot generation component of Qiskit#1203 but updates the code to wrap tooltip in addition to label which was what the original bug reported. In 0.16.0 we should investigate adding a flag to control the escaping behavior of the function to either decide to wrap values in quotes or not.
This commit fixes a regression introduced in Qiskit#1203. That PR was attempting to fix missing character escaping in some string fields but it was doing so too eagerly. This would result in invalid dot files being produced for users upgrading from rustworkx < 0.15.0 that were wrapping strings in quotes as needed previously. For example if you were setting `'color="#aaaaaa"'` previously before this would become `color="\"#aaaaaa\""` after Qiskit#1203. In order to quickly release a 0.15.1 this commit reverts the dot generation component of Qiskit#1203 but updates the code to wrap tooltip in addition to label which was what the original bug reported. In 0.16.0 we should investigate adding a flag to control the escaping behavior of the function to either decide to wrap values in quotes or not.
This commit fixes a regression introduced in #1203. That PR was attempting to fix missing character escaping in some string fields but it was doing so too eagerly. This would result in invalid dot files being produced for users upgrading from rustworkx < 0.15.0 that were wrapping strings in quotes as needed previously. For example if you were setting `'color="#aaaaaa"'` previously before this would become `color="\"#aaaaaa\""` after #1203. In order to quickly release a 0.15.1 this commit reverts the dot generation component of #1203 but updates the code to wrap tooltip in addition to label which was what the original bug reported. In 0.16.0 we should investigate adding a flag to control the escaping behavior of the function to either decide to wrap values in quotes or not.
Fixes #750
Updated the fn attr_map_to_string function
I can add a separate function to handle the escape values if needed
Will finish up docs and tests once the solution is satisfactory