-
Notifications
You must be signed in to change notification settings - Fork 86
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
Support multi-word mermaid commands #91
base: master
Are you sure you want to change the base?
Conversation
To support words (paths, arguments) containing whitespace, shell-compatible quotes or a list of strings can be supplied.
sphinxcontrib/mermaid.py
Outdated
mm_args += ['-i', tmpfn, '-o', outfn] | ||
mm_args.extend(self.builder.config.mermaid_params) |
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.
@slanzmich isn't the same than directly add the extra params in the mermaid_params
config?
mermaid_params = [...]
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.
Users cannot add the -i
/ -o
commands, because they depend on the current input file and the temporary directory.
It is however not a problem if users specify these parameters in their command, because they will be overwritten by the ones we append. I.e.
npx mmdc -i input_1.md -o output_1.png -i input_2.md -o output_2.png
will convert input_2.md
to output_2.png
.
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.
Sorry, I misread your comment and confused mermaid_params
with mermaid_cmd
option.
Adding any additional parameters to mermaid_params
does not work for my use case, because the -i
/ -o
options are inserted first, such that they are passed to the wrong command (npx
instead of mmdc
in my case).
In fact, the -i
/ -o
options should be added after the mermaid_params
to prevent users from accidentally messing with these parameters. If that is changed, my change would be optional in the sense that I could also configure
mermaid_cmd = "npx"
mermaid_params = ["--no-install", "mmdc"]
However, the fact that this works is an implementation detail and feels wrong. The mermaid_cmd
is actually npx --no-install mmdc
, the arguments are no arguments to mermaid
.
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 pushed a few more commits. Let me know what you think and if I should squash any of them.
I guess the order may be important, so could you also update the readme to mention that |
To support words (paths, arguments) containing whitespace,
shell-compatible quotes or a list of strings can be supplied.
This fixes issue #89 in a more robust way than #79.