SMB Script which collect remote files, mount locations, unmount locations, and show directory file tree

Question

I've written a script that is able to accomplish the following:

1. mount/unmount a remote SMB location using the subprocess module
2. List the file contents of a specified directory
3. copy a single file out from a remote directory to a specified destination folder.
4. List subdirectories within a mounted SMB location.

I've done my best to make as much of my functionality reusable within functions, but there's a part of me that worries functions like selectSingleFile() is getting a little convoluted.

Here is my code:

import os
import shutil
import subprocess
from os import walk
from shutil import copy2
from glob import glob

def mount(remote_dir, local_dir):
    """Mounts a remote directory and chooses a local file in which to mount the directory against"""
    retcode = subprocess.call(["/sbin/mount", "-t", "smbfs", remote_dir, local_dir])

def unmount(local_dir):
    """Unmounts the local SMB directory"""
    retcode = subprocess.call(["/sbin/umount", local_dir])

def getFileContents(directory):
    """Creates a list of files within a directory"""
    f = []
    for (dirpath, dirnames, filenames) in walk(directory):
        f.extend(filenames)
        break
    return f

def printChoices(local_dir):
    """Used to print a list of numbered list of files in a directory"""
    num = 1
    a = getFileContents(local_dir)
    for item in a: 
        print(str(num) + ".", item)
        num +=1

def listDirectory(directory):
    return glob(directory)

def selectSingleFile(local_dir, destination):
    '''Used to get the contents of a directory and select a single file to copy to a destination
    --requires printChoices() and getFileContents()
    '''
    a = getFileContents(local_dir)
    printChoices(local_dir)
    b = int(input("please select your choice: "))
    copy2(local_dir+(a[b-1]), destination)

''' The following was an example usage of the functions above. It allowed for mounting of a remote
file location, within the work network, and a listing/transfer of a single file within the directory.
'''
local_dir = '/Users/ParanoidPenguin/smb/' #Specifies the local directory to mount
destination = '/Users/ParanoidPenguin/Desktop/JPEGs'#For copy2, destination where file will be stored
smb_location = '//[email protected]/Documents' #smb location to be mounted



mount(smb_location, local_dir)

selectSingleFile(local_dir, destination)


input("Press any key to unmount: ")
unmount(local_dir)

Answer 1

I didn't try to run your program. So some things may not work.

1. Imports. There is no need to do them twice like what you did with os and shutil. Just write import os or import shutil and then use them like os.walk and shutil.copy2. Same goes for glob.
2. PEP 8.

   • Surround top-level function and class definitions with two blank lines.

   • Limit all lines to a maximum of 79 characters. For flowing long blocks of text with fewer structural restrictions (docstrings or comments), the line length should be limited to 72 characters.

   • Function names should be lowercase, with words separated by underscores as necessary to improve readability.

   • Inline comments should be separated by at least two spaces from the statement. They should start with a # and a single space.

   • Docstrings should be surrounded by """. In selectSingleFile you used '''.
3. Naming conventions. Variables should have self-explanatory names. Your a, b, f don't say anything. getFileContents probably should be called something like get_files_list or get_dir_contents?

4. Type hints. With type hints you can help other people and yourself to understand what type of data your functions get as input and what type of output is going to be produced. For example, signature of your getFileContents will look like this:

   def getFileContents(directory: str) -> List[str]:
       # Your code...
   

   Don't forget to include from typing import List in the beginning.

5. You have not used variables called retcode. You can safely delete them. Also listDirectory is never used.
6. Some of your docstrings say for example: "Used to print...". Why not just write "Prints..."?
7. Instead of using subprocess.call you could use subprocess.check_call. So if something goes wrong you will get a CalledProcessError instead of nothing.

8. Your mount and unmount are quite similar. We could use partials. (I hope I didn't go too far with this). For example:

   import subprocess
   from functools import partial    
   
   
   def check_call(*args) -> None:
       subprocess.check_call(args)
   
   
   mount = partial(check_call,
                   '/sbin/mount',
                   '-t',
                   'smbfs')
   mount.__doc__ = ('Mounts a remote directory '
                    'and chooses a local file ' 
                    'in which to mount the directory against')
   
   unmount = partial(check_call,
                     '/sbin/umount')
   unmount.__doc__ = 'Unmounts the local SMB directory'
   

   Probably, something should be done about hard-coded call arguments but I'm not sure what.

9. In getFileContents instead of creating an empty list and then filling it we could create it by list comprehension. I propose two versions:

   def getFileContents(directory: str) -> List[str]:
       return [file
               for *_, files_list in os.walk(directory)
               for file in files_list]
   

   Note the *_. We use _ for throwaway variables. And with the asterisk we say that there are multiple throwaway variables like in your case: dirpath and dirnames.
   Another way is to use itertools.chain:

   def getFileContents(directory: str) -> List[str]:
       files_lists = [files_list
                      for *_, files_list in os.walk(directory)]
       return list(itertools.chain(*files_lists))
   

10. In selectSingleFile it would be good to add checking input. Take a look at the first answer here.
11. In the same function you calculate list of files twice. First, in the function itself and the second is in the printChoices. So, instead of passing local_dir to printChoices, pass already calculated list of files (a - how you call it)

12. Instead of joining paths with a + use os.path.join By this point your function should look like this:

    def select_single_file(local_dir: str,
                           destination: str) -> None:
        """
        Gets the contents of a directory
        and selects a single file to copy to a destination.
        """
        files = get_file_contents(local_dir)
        print_choices(files)
    
        # TODO: add checking
        file_index = int(input("Please select your choice: "))
        file_path = os.path.join(local_dir,
                                 files[file_index - 1])
        shutil.copy2(file_path,
                     destination)
    

13. Finally, you have a counter variable in printChoices. Why not use enumerate? So, you will have:

    def print_choices(files: List[str]) -> None:
        """Prints a numbered list of files in a directory"""
        for index, filename in enumerate(files, start=1):
            print(str(index) + ".", filename)  
    

P.S.: "Please select your choice: " This is tautology.

Edit: You asked if it would be a good idea to expand functions to deal with exceptions. If you want to ask user to enter paths until he gives you correct ones then yes. But if you just want to throw an error if the entered path by user is incorrect, you should test your functions with valid and invalid input data. If for invalid input data you get a traceback from which it is not clear what goes wrong, you could add catching exceptions in those functions. I think you will need to use something from os.path.isfile, os.path.isdir, os.path.ismount and os.path.exists. Check the docs.