4 ms·

I am not a huge fan of copying the shell language wholesale and wrapping inside a macro. Since macros can execute arbitrary code, this makes me feel uneasy that

I am not a huge fan of copying the shell language wholesale and wrapping inside a macro. Since macros can execute arbitrary code, this makes me feel uneasy that the strings are just executed within a shell context, with all the appropriate, bug-prone, expansion done by the shell. Seeing "ls /nofile || true;" makes me worry that "||" is actually passed to the shell wholesale. There's also no transparency about how the binary names are resolved. I much prefer an approach more integrated with the language, like Plumbum: https://plumbum.readthedocs.io/en/latest/local_commands.html#guide-local-commands https://plumbum.readthedocs.io/en/latest/local_commands.html... This no longer looks like the POSIX shell, but instead clearly integrates the good parts directly into the language, even if some complexity bubbles through. I don't have to worry that "grep["world"] < sys.stdin" is piped into an actual shell, because it gets converted into an AST on the way to execution.

IshKebab 6y ago
I agree, this is a misguided idea. The whole point of not using Bash is that you don't have to use it's terrible design and syntax surely?
- foota 6y ago
  This only seems to use the good parts of the shell, easy piping and redirection, while dropping the language for logic.
  - IshKebab 6y ago
    It also appears to use some bad parts, e.g. command line switches and unquoted arguments.
    - rustshellscript 6y ago
      Unquoted argument is not an issue here, see some examples here: https://github.com/rust-shell-script/rust_cmd_lib/issues/10 https://github.com/rust-shell-script/rust_cmd_lib/issues/10
papaf 6y ago
Since macros can execute arbitrary code, this makes me feel uneasy that the strings are just executed within a shell context, with all the appropriate, bug-prone, expansion done by the shell. Its a shame you didn't bother to look at the source code before criticizing. Someone put a lot of work into this library and its actually pretty cool. The package parses the code in the macros [1] and then calls 'std:Process::Command' [2] which, I believe, does not execute a subshell by default. [1] https://github.com/rust-shell-script/rust_cmd_lib/blob/master/crates/cmd_lib_core/src/parser.rs#L149 https://github.com/rust-shell-script/rust_cmd_lib/blob/maste... [2] https://github.com/rust-shell-script/rust_cmd_lib/blob/master/crates/cmd_lib_core/src/process.rs#L313 https://github.com/rust-shell-script/rust_cmd_lib/blob/maste...
- viraptor 6y ago
  I don't think the parent said this is not parsed well, or at least I didn't read it that way. I share the feeling that you see that code and unless you know the implementation it's not clear what shell brokenness is carried over and what isn't. And which shell and version is being emulated. It's much easier to set expectations with a new syntax that's also easier to document than "what to expect of this macro".
  - rattray 6y ago
    These sound like easy documentation fixes - maybe open an issue?
  - rhn_mk1 6y ago
    Thank you, that's indeed what I meant. The other, related issue is that reimplementing pieces of the shell DSL duplicates what can be done in the parent language. Taking conditional return value as an example: "ls /nofile || true;" In this case I don't really want to be given the option to use bash syntax for this. That would encourage the usage of shell idioms for "tricks" like control flow, which are another annoying part of the shell (I can never remember them). I would much prefer if there was a nice way to do that kind of things idiomatically in the parent language, and no other choice. E.g. to ignore the return value I would find it much nicer to be forced to do sth like this instead: let _ = ls('nofile');
cheez 6y ago
Oh this plumbum is nice.
rustshellscript 6y ago
Author here, thanks for your feedback. This library is calling std::process::command underneath without any shell dependency, which I believe is the same as plumbum’s run* APIs and I should make it clearer in the docs. This plumbum has its own “cp”, “cat” .., which is similar to shelljs and it looks like a lot of people like this idea which can also be supported by this library in the future.

IshKebab 6y ago

I agree, this is a misguided idea. The whole point of not using Bash is that you don't have to use it's terrible design and syntax surely?

foota 6y ago
This only seems to use the good parts of the shell, easy piping and redirection, while dropping the language for logic.
- IshKebab 6y ago
  It also appears to use some bad parts, e.g. command line switches and unquoted arguments.
  - rustshellscript 6y ago
    Unquoted argument is not an issue here, see some examples here: https://github.com/rust-shell-script/rust_cmd_lib/issues/10 https://github.com/rust-shell-script/rust_cmd_lib/issues/10

papaf 6y ago

Since macros can execute arbitrary code, this makes me feel uneasy that the strings are just executed within a shell context, with all the appropriate, bug-prone, expansion done by the shell. Its a shame you didn't bother to look at the source code before criticizing. Someone put a lot of work into this library and its actually pretty cool. The package parses the code in the macros [1] and then calls 'std:Process::Command' [2] which, I believe, does not execute a subshell by default. [1] https://github.com/rust-shell-script/rust_cmd_lib/blob/master/crates/cmd_lib_core/src/parser.rs#L149 https://github.com/rust-shell-script/rust_cmd_lib/blob/maste... [2] https://github.com/rust-shell-script/rust_cmd_lib/blob/master/crates/cmd_lib_core/src/process.rs#L313 https://github.com/rust-shell-script/rust_cmd_lib/blob/maste...

viraptor 6y ago
I don't think the parent said this is not parsed well, or at least I didn't read it that way. I share the feeling that you see that code and unless you know the implementation it's not clear what shell brokenness is carried over and what isn't. And which shell and version is being emulated. It's much easier to set expectations with a new syntax that's also easier to document than "what to expect of this macro".
- rattray 6y ago
  These sound like easy documentation fixes - maybe open an issue?
- rhn_mk1 6y ago
  Thank you, that's indeed what I meant. The other, related issue is that reimplementing pieces of the shell DSL duplicates what can be done in the parent language. Taking conditional return value as an example: "ls /nofile || true;" In this case I don't really want to be given the option to use bash syntax for this. That would encourage the usage of shell idioms for "tricks" like control flow, which are another annoying part of the shell (I can never remember them). I would much prefer if there was a nice way to do that kind of things idiomatically in the parent language, and no other choice. E.g. to ignore the return value I would find it much nicer to be forced to do sth like this instead: let _ = ls('nofile');

cheez 6y ago

Oh this plumbum is nice.

rustshellscript 6y ago

Author here, thanks for your feedback. This library is calling std::process::command underneath without any shell dependency, which I believe is the same as plumbum’s run* APIs and I should make it clearer in the docs. This plumbum has its own “cp”, “cat” .., which is similar to shelljs and it looks like a lot of people like this idea which can also be supported by this library in the future.