Skip to content

Conversation

robothor
Copy link
Contributor

@robothor robothor commented May 2, 2018

Hi! Thanks for starting this tool!

We needed to expose the ability to set environment variables on the underlying Cmd object. This PR contains the minimal set of changes we needed to get this to work and a basic test for this functionality.

As per the normal os/exec.Cmd, if the Env is nil (i.e. not set) then the current process' environment will be used.

I didn't see any contributor guidelines, so this is best-effort -- please let me know if you need me to make any changes.

@coveralls
Copy link

coveralls commented May 2, 2018

Coverage Status

Coverage remained the same at 100.0% when pulling 924ae7f on robothor:env into bcc8acb on go-cmd:master.

@daniel-nichter
Copy link
Member

It maintains backwards-compatibility, so I'll merge it. It makes me realize that I'll need to redesign this pkg a little and release a 1.1 series. Question is: what if someone wants to set/change Cmd.Dir or Cmd.ExtraFiles in the base os/exec.Cmd? This pkg can't and shouldn't duplicate all the base Cmd fields, but it shouldn't block the user form setting/changing them, either. So a better design for v1.1 would be to expose the base Cmd directly so users have full control of that while still getting the benefits of this wrapper.

That can wait for later. For now, adding Env is an acceptable duplication of the base Cmd.Env, and test coverage remains 100%. Thanks for the PR!

@daniel-nichter daniel-nichter merged commit 39e09e2 into go-cmd:master May 3, 2018
@robothor robothor deleted the env branch May 3, 2018 12:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants