fix: handling of d2-flags #48
Conversation
|
thank you for the improved handling and PR! Will review and hopefully can merge this in! |
|
Thank you, just a heads up, I did change the |
d2-mode.el
Outdated
| @@ -117,7 +117,7 @@ | |||
| (cmd (concat (shell-quote-argument d2-location) | |||
| " " temp-file | |||
| " " (org-babel-process-file-name out-file) | |||
| " " d2-flags))) | |||
| " " (string-join d2-flags " ")))) | |||
There was a problem hiding this comment.
Do we need to require subr-x to support the string-join function?
There was a problem hiding this comment.
Hey @terlar . I think it's fine breaking compatibility here. I think this is going to be a bump into a new version. Probably 0.2 to signify breaking. Along with #47. Maybe some linting updates as well.
I'm super swamped today/tomorrow, but I will try to review this over the weekend/later half of the week. Sorry for the delay and thank you again for the contribution.
string-join is built in Emacs 25. Think we are fine. We can also just add the function directly:
;;;###autoload
(defsubst string-join (strings &optional separator)
"Join all STRINGS using SEPARATOR.
Optional argument SEPARATOR must be a string, a vector, or a list of
characters; nil stands for the empty string."
(declare (pure t) (side-effect-free t))
(mapconcat #'identity strings separator))I'd prefer to not require a library for something as simple as string-join but i'm not entirely opposed to it either.
There was a problem hiding this comment.
Thank you, no stress. Take your pace. Yeah, if we have to require a library we might as well just do mapconcat, perhaps we also want to shell escape all arguments as well anyway.
There was a problem hiding this comment.
sounds good. will get this merged in asap. Something came up this weekend and going to have to delay my review until mid next week. my apologies. I'm looking forward to the review.
There was a problem hiding this comment.
No worries, then I will have time to update with mapconcat instead
There was a problem hiding this comment.
https://github.com/andorsk/d2-mode/actions/runs/9472621470/job/26295213297?pr=48#step:5:59 <- string join needs to be added in, per this. I will add a suggestion here for string-join. Lmk if you're ok wth it
| (progn | ||
| (d2-browse-file output)) | ||
| (progn | ||
| (d2-browse-file output)) | ||
| (progn | ||
| (display-buffer (find-file-noselect output t)))))) | ||
|
|
There was a problem hiding this comment.
Add the following:
| ;;;###autoload | |
| (defsubst string-join (strings &optional separator) | |
| "Join all STRINGS using SEPARATOR. | |
| Optional argument SEPARATOR must be a string, a vector, or a list of | |
| characters; nil stands for the empty string." | |
| (declare (pure t) (side-effect-free t)) | |
| (mapconcat #'identity strings separator)) |
There was a problem hiding this comment.
Note: tested this and it seemed to work. Let's get this in and merge to main. After #47 , bump version.
There was a problem hiding this comment.
I went for mapconcat directly instead, since then I can specify the separator and can also apply the shell-quote-argument to all arguments which is good practice.
|
@terlar, thanks for being patient. Can we get string-join added in above, and make sure the builds above are fixed? Should be good to go after that. |
They were only passed to the org babel execution and not to the regular compile file command. This also converts it to a list instead of a string in order to better fit the call-process function.
|
note: ci is breaking due to some weird upstream issues, but one from each os ( mac, windows, and ubuntu ) all passed, so I think this is fine. New issue tagged here: #49 for this. LGTM, and approved. |
They were only passed to the org babel execution and not to the regular compile file command.
This also converts it to a list instead of a string in order to better fit the call-process function.