-
Notifications
You must be signed in to change notification settings - Fork 2
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
Feature/add icon packs #5
Conversation
Hi @owynrichen , could you please fix the EOL before reviewing? It changes too many things now make it harder to review |
Can do, let me take a look and update it. |
Got it done! Sorry for the delay. It should be ready for review now. |
It still contains lots of files I think might not need to be changed 🤔 Could you please use |
Hey, I appreciate the POV. The changes in /tasks were to enable the passing of choice for pty from the command-line vs. forcing it to True always, to aide in execution on Windows (and corresponding documentation referencing the option). It still defaults to True so it's a no-op for folks generally. The other changes are related to the additions themselves and adjustments to unit tests to match. The updates to the pre-commit-hook happened automatically when running the contributing documentation. I'm happy to revert those if you'd prefer. |
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.
The idea looks good. left some suggestions. Thanks for helping out!
I just checked again. Somehow the last time it did not reflect the changes 🤔 Thanks for updating! |
Awesome, thanks for the feedback! I've updated things based on your recommendations. |
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.
Most of the remaining things are just nitpicks. We're really close to merge. Thanks for helping out!
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.
one last nitpick and then we can merge it!
) # for the None case | ||
self.icon_packs.update(config_packs) | ||
|
||
"""Add mermaid diagram markdown codeblocks.""" |
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.
Let's move it to class docstring
we might need to take care of the blacken-docs version as well |
d00bb97
to
755777f
Compare
…h mermaid via extension configuration Some newer diagrams (like architecture-beta) leverage icons that can be customized via mermaid.registerIconPacks() https://mermaid.js.org/config/icons.html, this exposes that capability via extension configuration and adds some tests
755777f
to
6bd4a64
Compare
feat(markdown_mermaidjs.py,-test_markdown_mermaid.py,-some-tasks): adds the ability to register icon packs with mermaid via extension configuration
Some newer diagrams (like architecture-beta) leverage icons that can be customized via mermaid.registerIconPacks() https://mermaid.js.org/config/icons.html, this exposes that capability via extension configuration and adds some tests.
Types of changes
Description
In addition to the registerIconPacks() work (with tests/etc), I also did some refactoring.
It also adds the ability to decide to bypass pty for tasks that require it so they can run on Windows. The big exception is the git.commit task doesn't work properly as prompt_toolkit can't seem to detect when running in cmd.exe (even when it is). It functions as expected in Ubuntu, though.
Checklist:
inv style
locally to ensure all linter checks passinv test
locally to ensure all test cases passinv secure
locally to ensure no major vulnerability is introducedSteps to Test This Pull Request
The unit tests and documentation additions should cover the primary cases. Basically pass in an extension config
with icon_packs declared that match the { name : url } pattern i.e.:
and it should add corresponding mermaid.registerIconPacks() calls in the script module declaration i.e.:
Expected behavior
The unit tests assert this behavior, specifically:
test_add_mermaid_script_and_tag_with_icons()
Related Issue
Additional context