Skip to content
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

upgrade os.system to subporcess.popen method #1066

Merged
merged 4 commits into from Aug 31, 2021

Conversation

denghuilu
Copy link
Member

Upgrade the os.system to subporcess.popen method. According to the doc of os.system method:

The subprocess module provides more powerful facilities for spawning new processes and retrieving their results; using that module is preferable to using this function. See the Replacing Older Functions with the subprocess Module section in the subprocess documentation for some helpful recipes.

In general, subprocess.Popen() is super-set of os.system().

@codecov-commenter
Copy link

codecov-commenter commented Aug 30, 2021

Codecov Report

Merging #1066 (31161a2) into devel (8dbc211) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##            devel    #1066   +/-   ##
=======================================
  Coverage   75.16%   75.16%           
=======================================
  Files          87       87           
  Lines        6959     6959           
=======================================
  Hits         5231     5231           
  Misses       1728     1728           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8dbc211...31161a2. Read the comment docs.

@@ -17,6 +18,16 @@ def _file_delete(file) :
if os.path.exists(file):
os.remove(file)

def _subprocess_run(command):
popen = sp.Popen(command, shell=True, stdout=sp.PIPE, stderr=sp.STDOUT)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer setting shell to False and using command.split() instead of command. Using a shell is not safe.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@amcadmus amcadmus merged commit 37fe14c into deepmodeling:devel Aug 31, 2021
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.

None yet

4 participants