Break out utils.run_shell_command to new class
#526
Conversation
elebow
force-pushed
the
refactor-run_shell_command-as-class
branch
from
874e6bb
to
d105606
Compare
|
The test builds are failing because of (I think) #527. |
Codecov Report
@@ Coverage Diff @@
## master #526 +/- ##
==========================================
+ Coverage 18.65% 19.31% +0.66%
==========================================
Files 31 32 +1
Lines 5061 5079 +18
Branches 1282 1282
==========================================
+ Hits 944 981 +37
+ Misses 4090 4071 -19
Partials 27 27
Continue to review full report at Codecov.
|
elebow
force-pushed
the
refactor-run_shell_command-as-class
branch
2 times, most recently
from
28f26ac
to
17ac42f
Compare
Ongoing refactoring efforts have been trying to introduce new packages. All packages must be listed in the packages argument to `setuptools.setup()`. `setuptools.find_packages()` provides a convenient way to list all packages without manually updating a list.
elebow
force-pushed
the
refactor-run_shell_command-as-class
branch
from
17ac42f
to
781cc06
Compare
| def modified_env(self): | ||
| env = os.environ.copy() | ||
|
|
||
| if self.extra_env: |
There was a problem hiding this comment.
Would it make sense to use a ChainMap for this?
There was a problem hiding this comment.
@jstoja This would be a clever improvement to the current code. I've opted to merge this PR as it is (or rather through the rebase at #577), just because it primarily refactors existing code without changing any other logic.
If you are up for adding the ChainMap improvement to this class, I would love to review it and add it to master.
Description of proposed changes
No functional changes. Rewrite
utils.run_shell_commandas a class.This obsoletes #513. If this one is merged first, #513 should be closed.
Related issue(s)
none
Testing
Unit tests included.