Conversation
seddonym
left a comment
seddonym
left a comment
There was a problem hiding this comment.
This is looking great, thanks!
As you mention in the issue, it would be great to add (i) documentation and (ii) tests.
Re. tests, we should have coverage of the different parameters passed in in tests/unit/application/test_use_cases.py. I also think it's worth a test that output redirection is working, possibly that could be added to the test recipe in the justfile.
|
ready for final review |
seddonym
left a comment
seddonym
left a comment
There was a problem hiding this comment.
Nice work.
Just a few quick tweaks before this is ready to go. If you're feeling like it, I also think it would be a good moment to improve the formatting of the dot output.
| viewer.view(dot) | ||
| output = mock_stdout.getvalue() | ||
|
|
||
| # Verify it's DOT content |
There was a problem hiding this comment.
I think it would be useful to assert against the entire DOT content here, rather than just doing a spot-check.
There was a problem hiding this comment.
Also, I notice the dot content is not formatted nicely (my fault 😄 ). Non-blocking, but if it's easily done maybe it would be worthwhile correcting that, now people may actually view the dot content.
There was a problem hiding this comment.
I propose to fix that in another PR (I plan to work on a mermaid output too)
|
maybe you should approve the github action workflow execution to check if there is any CI issue? |
seddonym
left a comment
seddonym
left a comment
There was a problem hiding this comment.
Generally looking good, but just test-output-redirection needs to be compatible with Mac OS and Linux. (Even better, Windows, but don't worry if not.)
2 participants
Closes: #30
just check.CHANGELOG.rst.AUTHORS.rst.