Conversation
phil-opp
left a comment
There was a problem hiding this comment.
Thanks for the separate PR! Am I understanding it correctly that this new version does not support any quoting in the normal build string anymore, compared to #1098? Instead, we now have the option to specify the args as list, right?
The example in the PR description seems outdated, it's a list of exec/args pairs now, right?
| CommandConfig::WithArgs(configs) => { | ||
| let mut cmd_vec = vec![]; | ||
| for CommandWithArgs { exec, args } in configs { | ||
| let mut cmd = Command::new(exec); |
There was a problem hiding this comment.
We should probably do the same pip/uv replacement here.
|
The new syntax seems quite verbose, I'm not sure if this is worth it. I think I liked the previous idea of supporting quoted substrings more. How about we just use a crate such as |
Yes. Because string processing is complex and some corner cases quotation marks might not be considered.😭 A list makes it easy to handle and it is also commonly used in configuarion(like some settings for the VSCode) |
|
I don't think we should use list as it would break what people know from configuration like docker / GitHub action... |
This crate looks good. |
Signed-off-by: drindr <dreamchancn@qq.com>
4a2afd1 to
7497ecb
Compare
|
Reverted to use quoted arguments |
Docker provided the list form as well. The implementation of the GitHub action seems to depend on a specific shell and execute the command with the shell. But in current Dora's build, it is not relied on any shell, which makes the functionalities of the build command limited. |
ready to merge @haixuanTao |
Allow argument with space in build command.
This PR is compatible with the old single string command.