Added escaping of vertical bar character in annotation labels #8610
Conversation
…al bar character in annotation labels to ensure it is not treated as field separator of record-based nodes
…notation_label_of_return_type` with a variant for testing return type using syntax `X | Y`
…ore" comment` pre-commit hook error
|
How to proceed with the failing tasks? |
This comment has been minimized.
This comment has been minimized.
Pierre-Sassoulas
added
Bug 🪲
pyreverse
|
@Pierre-Sassoulas Ok, at the time I posted my comment on how to proceed with the failing tasks the status of the checks was looking different. I then committed another change to get rid of the failing tasks. Now, as you already mentioned, many tests are failing because of the escaping implemented in a method that seems to be used in other printers as well. Didn't know that and the reference checks in VSCode did not reveal this. Hopefully I can find another solution to fix the missing escaping. |
…e handling is now part of the printer as it is DOT language specific
…d diadefs and inspector tests to also cover nullable patterns
for more information, see https://pre-commit.ci
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #8610 +/- ##
==========================================
- Coverage 95.91% 95.81% -0.11%
==========================================
Files 174 174
Lines 18416 18321 -95
==========================================
- Hits 17664 17554 -110
- Misses 752 767 +15
|
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Thank you for providing a fix for this issue!
Looks good from my side, there is only some whitespace in the HTML output that should be removed again.
You also have to provide a towncrier fragment which we use to generate our changelog. Simply run:
towncrier create 8603.bugfixand then add a short description of the issue that was solved, and a reference to the issue:
<your description>
Closes #8603As a side note, if you want to contribute to pyreverse again:
At least for class diagrams, it is most of the times easier to create a functional test file instead of expanding the existing test data under tests/data. The latter has the drawback (as you probably noticed 😁) that a lot of files - possibly for output formats unaffected by this change - have to be adapted, and the tests for test_diadefs and test_inspector have to be updated as well.
For this PR you can leave the tests as they are, but this could save you some time in the future!
The whitespaces are produced by the file generators. I just copied the generated files from my temp directory to the data folder. Anyway, I removed it as requested, looks better this way for sure.
Yep, done in commit 28d168a
I tried out various approaches to this and to be honest, the functional folder scared me of a bit as I have never used such a approach 😁. My thinking here was to provide an entry point for testing other parts of the code where nullable syntax can be used like method arguments. I initially got "creative" with extending Anyway, great to see the PR being accepted. |
This comment has been minimized.
This comment has been minimized.
… hint as suggested in the review process
|
@DudeNr33 Do you normally resolve the open conversations resulting from the review process or does the PR creator do that? I would assume the former. |
There was a problem hiding this comment.
We do not have a strict rule that all conversations have to be resolved before a PR can be merged. I normally also don't resolve conversations as - at least for small PRs - I find it easier to see what was discussed and what the result was. But that is personal preference.
|
Who merges the PR? |
|
The backport to To backport manually, run these commands in your terminal: # Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-maintenance/2.17.x maintenance/2.17.x
# Navigate to the new working tree
cd .worktrees/backport-maintenance/2.17.x
# Create a new branch
git switch --create backport-8610-to-maintenance/2.17.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 5d82d7bb6b94a3a6a4d7a4861262519e62916881
# Push it to GitHub
git push --set-upstream origin backport-8610-to-maintenance/2.17.x
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-maintenance/2.17.xThen, create a pull request where the |
|
One of the maintainers that have the required rights to merge. Typically one of those that also reviewed the changes. Congratulations on becoming a contributor! 😊 |
… to ensure it is not treated as field separator of record-based nodes, see discussion in #8603.
Type of Changes
Description
Currently the DOT printer produces a label that matches exactly the type hint. In case of nullable return types like
int | Nonethe|character (vertical bar) has the special meaning of a fiel separator for record-based nodes in DOT language. Hence it must be escaped as mentioned in the docs:Refs #8603
Closes #8603