Skip to content
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

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

slanzmich
Copy link

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.

To support words (paths, arguments) containing whitespace,
shell-compatible quotes or a list of strings can be supplied.
Comment on lines 174 to 175
mm_args += ['-i', tmpfn, '-o', outfn]
mm_args.extend(self.builder.config.mermaid_params)
Copy link
Owner

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 = [...]

Copy link
Author

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.

Copy link
Author

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.

Copy link
Author

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.

@mgaitan
Copy link
Owner

mgaitan commented Sep 1, 2022

I guess the order may be important, so could you also update the readme to mention that mermaid_cmd could be a list?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants