refactor redundant code into a function #339
Conversation
internal/multigitter/cmd.go
Outdated
| "os/exec" | ||
| ) | ||
|
|
||
| func prepareScriptCommand(ctx context.Context, repo scm.Repository, workDir string, scriptPath string, arguments []string, dryRun bool) (cmd *exec.Cmd) { |
There was a problem hiding this comment.
At first, I thought we were able to reuse even more logic, but looking at it, it seems to not be the case.
Looking at the broken-out logic now, a lot of parts are just passed to the exec command without any logic, or extra config, which I don't think justifies the generalization. What I think could be useful to share is the setting of the environment variables that should be the same between run/print (REPOSITORY + the os environment).
func scriptEnv(repo scm.Repository) []stringThoughts?
There was a problem hiding this comment.
Keeping the env vars identical is definitely a great idea - I thought the same.
About the generalization ... yes, it's just minor, I agree.
Still, I would argue it's a good idea since it also gives a hint that 'print' and 'run' have much in common.
There was a problem hiding this comment.
Overall I would argue, the broken-out logic might not be the strongest candidate for applying the DRY principle, but still, it helps to navigate/to remind that 'print' and 'run' have should have something in common.
PS: since run and print have different types, I was not able to use the same receiver, which would have simplified the parameter lists. I thought, it might be an idea to use type inheritance (see https://www.geeksforgeeks.org/inheritance-in-golang/), so that the print's and the run's receiver object would share e.g. the script path and the arguments.
That might lead to other shared functions as well (potentially).
Did you have tried that already, by any chance?
There was a problem hiding this comment.
I have not tried to break the logic into a shared struct-receiver. But that could potentially work. I would also be fine with this implementation, but the changes that would be needed are:
- Let each parameter be on a newline to make it easier to read.
- Remove
if dryRun {from this function, it can be exclusively in run.go
|
changes are implemented, as proposed. |
|
Thanks for the contribution :) |
|
Included in release v0.47.0 🎉 |
What does this change
As discussed in PR #337 , this is the refactoring to consolidate the code for preparing command execution.
What issue does it fix
n/a
Notes for the reviewer
None.
Checklist