Python function for 7zip compression

Question

I wrote a function in Python to call 7zip to compress a folder. The code works and is able to perform the task, however I do not know if this is the most optimal way. Are there any ways to improve the code, any redundant steps?

def Compress_7z(Initial_path,Container_Name,File_List, compression_level = -1,DAC = False): """Compression level = -1 standard, 0-9 compression level DAC : Delete After Compression - Remove File after being compressed """ try: chdir(Initial_path) except: return('Path does not exist') # write a listfile with open('list.txt', 'w') as f: for item in File_List: f.write("%s\n" % item) cmd = ['7z', 'a', Container_Name,'@list.txt'] if compression_level != -1 and compression_level <= 9: cmd.append('-mx{0}'.format(compression_level)) elif compression_level > 9 or compression_level < -1: return("Compression not standard: aborting compression") system = subprocess.Popen(cmd, stderr=subprocess.STDOUT, stdout=subprocess.PIPE,shell=True) while system.poll() == None: # .poll() will return a value once it's complete. time.sleep(1) if DAC: for f in File_List: remove(f) # Clean up from compression remove('list.txt') return(system.communicate()) 

Answer 1

Simply reading the code already has a few things which stand out to me (in general this all goes towards the style described in this document, which is standard to follow for Python code):

• Compress_7z should start lower-case and probably should also say compress_7zip. I'm agreeing that the literal "7" would be appropriate here, given the format name.
• DAC is a bad variable name, as you've to read the docstring to even begin to understand what this parameter is doing. A better name might be, well, delete_files_after perhaps.
• The rest of the parameters should also all be lower-case.
• Try and apply consistent formatting, or use an IDE or command-line tool to do it for you (right now there's no whitespace after some of the commas for example, also some of the quotes are single and some are double for no good reason).
• The docstring is not very standard, though at least it mentions the value range of the parameters, which is obviously good.

Next up, actual functionality:

Shelling out to the 7z utility is of course an option, although there are also pure Python libraries for this as far as I can see. The reason I point this out is that for one the command-line utility might not be installed and e.g. pip or a similar tool for Python might make it easier to handle the dependency.

Since you've already settled on it though, a few more points about the current implementation:

• chdir in a library function is almost certainly a bad idea. It's completely not expected that "compress these files" also means "change my current working directory". Instead make sure that the function works without by specifying file names reliably (that is, using absolute path names, or by specifying the working directory for the invoked process only).
• Using strings as error values is a big no. Use exceptions, or if you absolutely do not want to do that, a properly structured return value. Right now you couldn't distinguish the return value of the tool from the strings "Path does not exist" and that's generally to be avoided.
• If the compression level is a string the list.txt file will not be cleaned up. That's why you generally want to handle exceptions properly, look up how try, catch and finally work for that.
• Finally it seems like the subprocess.Popen invocation is too much work. If you don't have to communicate with the invoked process, simply run it and wait for the result. time.sleep is an extremely wasteful way of accomplishing the same. Instead I think you might just be able to use subprocess.run with the cwd argument (see above) and be done with it. You don't need shell=True either, since the invocation doesn't use any shell features at all.