feat: process supports string command #1361
Conversation
| } | ||
|
|
||
| func (r *Process) Run(name string, args ...string) contractsprocess.Result { | ||
| name, args = formatCommand(name, args) |
There was a problem hiding this comment.
I suggest we make this explicit by adding a RunShell method. The auto-detection is ambiguous. For instance, on Windows, running a binary inside c:\Program Files\App.exe would fail because the code treats the space as a shell separator.
It also creates inconsistent behavior. For instance, facades.Process().Run("ls | grep foo", "-la") will fail. Because a second argument is present, the logic skips the shell detection and tries to find an executable literally named 'ls | grep foo'
There was a problem hiding this comment.
I also considered adding a new function like RunShell, but I'm thinking if it's necessary to add a single function to support running string commands. I just want to omit /bin/sh -c, it's a bit heavy to add such a function.
c:\Program Files\App.exe is a good case, space may be contained in the route.
facades.Process().Run("ls | grep foo", "-la") doesn't trigger the formatCommand, so it's not related to this PR.
We can optimize the condition to if len(args) == 0 && str.Of(name).Contains("&", "|"), except space. Then it's simpler to run such commands:
There was a problem hiding this comment.
I know you are concerned about the verbosity of manually writing /bin/sh -c but the default Run method should remain raw and strict to avoid 'magic' inside it.
Go's standard library actually warns against this implicit shell invocation:
Unlike the "system" library call from C and other languages, the os/exec package intentionally does not invoke the system shell and does not expand any glob patterns or handle other expansions, pipelines, or redirections typically done by shells. The package behaves more like C's "exec" family of functions. To expand glob patterns, either call the shell directly, taking care to escape any dangerous input, or use the path/filepath package's Glob function. To expand environment variables, use package os's ExpandEnv.
There was a problem hiding this comment.
Or, if we need to provide this functionality, let’s add an annotation for it.
There was a problem hiding this comment.
Yes, Go has own standard, and our function is following that as default. I just want to provide an additional way to run string commands directly. It's very convenient in some situations.
There was a problem hiding this comment.
cmd /c "c:\Program Files\App.exe" is a valid command, so the space character can be added here.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #1361 +/- ##
==========================================
- Coverage 70.64% 69.99% -0.65%
==========================================
Files 286 284 -2
Lines 17664 17709 +45
==========================================
- Hits 12479 12396 -83
- Misses 4657 4777 +120
- Partials 528 536 +8 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
📑 Description
Support run string command directly:
facades.Process().Run("ls -a")instead offacades.Process().Run("/bin/sh", "-c", "ls -a").✅ Checks