-
Notifications
You must be signed in to change notification settings - Fork 10
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Make the single argument constructor inherit the env #113
Conversation
Codecov Report
@@ Coverage Diff @@
## gz-utils2 #113 +/- ##
=============================================
+ Coverage 79.67% 80.89% +1.21%
=============================================
Files 8 8
Lines 364 382 +18
=============================================
+ Hits 290 309 +19
+ Misses 74 73 -1
|
This adds functions for using collections with the environment. It allows for conveniently retreiving and setting the environment variables via a map, rather than individually. This also adds a clearenv and printenv for completeness. Signed-off-by: Michael Carroll <[email protected]>
Signed-off-by: Michael Carroll <[email protected]>
Signed-off-by: Michael Carroll <[email protected]>
4d1e541
to
d64ff9b
Compare
Signed-off-by: Michael Carroll <[email protected]>
Signed-off-by: Michael Carroll <[email protected]>
Signed-off-by: Michael Carroll <[email protected]>
Signed-off-by: Michael Carroll <[email protected]>
Signed-off-by: Michael Carroll <[email protected]>
Signed-off-by: Michael Carroll <[email protected]>
Signed-off-by: Michael Carroll <[email protected]>
Signed-off-by: Michael Carroll <[email protected]>
d64ff9b
to
a0a1c47
Compare
Windows CI is not happy |
And not in an expected place, I will take a look. |
Signed-off-by: Michael Carroll <[email protected]>
@azeey should be good now |
public: Subprocess(const std::vector<std::string> &_commandLine, | ||
const std::vector<std::string> &_environment = {}): | ||
gz::utils::EnvironmentMap _environment): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this signature, but would it does break API. Would it be possible to add Subprocess(const std::vector<std::string> &_commandLine, const std::vector<std::string> &_environment
)` that would turn around and call this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, ended up refactoring out my strings -> map -> strings functions so I can use the delegating constructor here.
One hitch is that it makes the map and vector constructors ambiguous:
// Could be either
Subprocess({}, {})
So now you have to explicitly do:
Subprocess({}, gz::utils::EnvironmentMap{});
Not a huge deal, but a little less clean imo.
Signed-off-by: Michael Carroll <[email protected]>
Signed-off-by: Michael Carroll <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Signed-off-by: Michael Carroll <[email protected]>
Now there are three ways to spawn a subprocess:
Subprocess({"env"})
- Spawn subprocess and inherit environment from current executableSubprocess({"env"}, {})
- Spawn subprocess with empty custom environmentSubprocess({"env"}, {"FOO=BAR"})
- Spawn subprocess with custom environment.Opening for comment, still needs tests + docs