3
\$\begingroup\$

Here's a function save_document(), that polls the pressing of a Extract PDF button in an external program and saves a tmp PDF file in a USB drive.

import time

import os
import shutil

def save_document():
    print("\n\nPreparing to save the document...", end = "\r")

    TEMP_PDF_DIR = "C:/Users/TUN/tp3/workspace/tmp" # temporary pdfs are stored in this directory

    while not os.listdir(TEMP_PDF_DIR): # wait until the "Extract PDF" button is pressed and a file will be created in directory TEMP_PDF_DIR
        time.sleep(0.5)
        continue

    file_path = "C:/Users/TUN/tp3/workspace/tmp" + "/" + os.listdir(TEMP_PDF_DIR)[0]

    shutil.move(file_path, find_drive_id()) # move the file in the USB drive directory 

    print("Document PDF was saved successfully.")

And the find_drive() function, which looks for a storage device with a given serial number and returns its path (path letter like D:, F:, etc.)

import wmi

def find_drive_id():
    local_machine_connection = wmi.WMI()

    DRIVE_SERIAL_NUMBER = "88809AB5" # serial number is like an identifier for a storage device determined by system

    '''.Win32.LogicalDisk returns list of wmi_object objects, which include information
    about all storage devices and discs (C:, D:)'''

    for storage_device in local_machine_connection.Win32_LogicalDisk():
        if storage_device.VolumeSerialNumber == DRIVE_SERIAL_NUMBER:
            return storage_device.DeviceID

    return False

Is there a better way to save a just created temporary PDF file?

Also, the polling loop is bothering me...I believe, there's a more elegant approach.

\$\endgroup\$

1 Answer 1

3
\$\begingroup\$

General comments

Personally the polling seems fine, a standard way of doing this is using watchdog. I'll just point out some bits and bobs I find odd about your code.


imports

A common way in Python is to split your imports into three sections: builtin modules, community modules and local. So this

import time

import os
import shutil

Should really be this

import time
import os
import shutil

Because time, os and shutil all live in the standard library.


descriptive function names

Naming this is hard, but it is very important to think carefully about what you name things; especially functions, classes and modules. The main reason why

save_document(): and find_drive_id()

Are bad names is because they do not do what they say they do. Save document does not save a document, it moves an already saved document to a different folder. Similarly find_drive_id does not find a drive ID but returns the path to a drive.


hardcoded paths

This is a 2 for 1 deal. First paths are better handled using the pathlib module (again in the standard library). In addition we are using this path multiple times, so it ought to be extracted into it's own global constant

import pathlib

DIRECTORY_TO_WATCH = pathlib.PureWindowsPath("c:/Users/TUN/tp3/workspace/tmp")

Similarly DRIVE_SERIAL_NUMBER = "88809AB5" should probably be a global constant as well.


docstrings

Triple quotes are usually reserved for docstrings. In addition this

'''.Win32.LogicalDisk returns list of wmi_object objects, which include information
about all storage devices and discs (C:, D:)'''

is more of a comment than a quote, and can be converted to #. In addition you should add docstrings explaining what each function does.


for else

This is really nitpicky, but we can write

for storage_device in local_machine_connection.Win32_LogicalDisk():
    if storage_device.VolumeSerialNumber == DRIVE_SERIAL_NUMBER:
        return storage_device.DeviceID
else:
    return False

Which I find clearer to read. If we do not break or return in the for loop the else clause is triggered. You can also think of the else as then.


if __name__ == "__main__":

Put the parts of your code that are the ones calling for execution behind a if __name__ == "__main__": guard. This way you can import this python module from other places if you ever want to, and the guard prevents the main code from accidentally running on every import.

Example code

Here is a mock-up of how a watchdog implementation could look. Note that I am on Linux and have no chance to check the details if everything is implemented correctly. However, it ought to be a good starting point =)

import time
import pathlib
import shutil

import wmi
from watchdog.observers import Observer
from watchdog.events import FileSystemEventHandler

DIRECTORY_TO_WATCH = pathlib.PureWindowsPath("c:/Users/TUN/tp3/workspace/tmp")
# serial number is like an identifier for a storage device determined by system
DRIVE_SERIAL_NUMBER = "88809AB5"
SLEEP_TIME = 5


class Watcher:
    DIRECTORY_TO_WATCH = DIRECTORY_TO_WATCH
    SLEEP_TIME = SLEEP_TIME

    def __init__(self):
        self.observer = Observer()

    def run(self):
        event_handler = Handler()
        self.observer.schedule(event_handler, self.DIRECTORY_TO_WATCH, recursive=True)
        self.observer.start()
        try:
            while True:
                time.sleep(self.SLEEP_TIME)
        except:
            self.observer.stop()
            print("Error")

        self.observer.join()


class Handler(FileSystemEventHandler):
    @staticmethod
    def on_any_event(event):
        if event.is_directory:
            return None

        elif event.event_type == "created":
            # Take any action here when a file is first created.
            # move the file in the USB drive directory
            file_path = pathlib.PureWindowsPath(event.src_path)
            shutil.move(file_path, get_usb_path())
            print("Document PDF was saved successfully.")


def get_usb_path():
    local_machine_connection = wmi.WMI()
    # Win32.LogicalDisk returns list of wmi_object objects, which include
    # information about all storage devices and discs (C:, D:)
    for storage_device in local_machine_connection.Win32_LogicalDisk():
        if storage_device.VolumeSerialNumber == DRIVE_SERIAL_NUMBER:
            return storage_device.DeviceID
    else:
        return False


if __name__ == "__main__":
    w = Watcher()
    w.run()
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.