5
\$\begingroup\$

I'm working on a project that requires me to open CSV files. It must check that the file is open and then return it to main. I've written a function to do so and would like to know how it looks, if I've written it well or how I can improve it:

menu.py


def print_menu_prompt():
    """
    Prints a menu prompt to the user
    
    Returns
        Nothing - just a prompt.
    """
    print("\n-----------------------------------------------------------------------------------")
    print("\nPlease enter the file path for the CSV you wish to calculate the hours worked for.")
    print("\nIn file explorer, navigate to the location of the CSV, click once on the file and copy the file path.")
    print("\nPaste the file path in exactly as it is - if the file path is incorrect then the program will not run.")
    print("\n-----------------------------------------------------------------------------------")

open_csv.py

from modules import menu



def open_file_prompt():
    """
    Display the prompt for the user to specify the path for the CSV file that they wish to open in the program.
    
    Returns
        The CSV file
    """
    # Infinite loop to open the CSV file, if file not found then it will loop again until the file is found.
    while True:
        menu.print_menu_prompt()

        timesheet_csv = input("\nPlease paste the path in now: ")

        # Try and except to check if the CSV file has opened successfully, if so then it is returned to main.
        try:
            with open(timesheet_csv, 'r') as file:
                if not file.closed:
                    print("\nThe CSV file opened successfully")
                    return timesheet_csv
                    break
        except FileNotFoundError:
            print(f"\nERROR: The specified file does not exist.")
        except IOError:
            print("\nAn error occured while trying to open the CSV file.")
\$\endgroup\$
1
  • 1
    \$\begingroup\$ Is the requirement just to open the file? Not actually reading and decoding it? \$\endgroup\$ Commented Sep 16 at 22:04

4 Answers 4

9
\$\begingroup\$

A few observations:

  • Your inclusion of leading newlines in prints is a little ugly to my eyes. If you really need an empty line, the previous print can just print an extra newline.
  • Your have an unnecessary f-string. print(f"\nERROR: The specified file does not exist.") should just be: print("ERROR: The specified file does not exist.")
  • Your doc string for open_file_prompt is inaccurate in that it claims to return the file, but it really returns the file name.
  • Breaking out of the loop after returning from it is unnecessary. Control flow will never reach the break.
  • In one error message you lead it with ERROR: but not in the other. This is inconsistent.
  • You likely want your error messages printed to standard error. This way if the end user wants to, they can redirect error messages. print("ERROR: The specified file does not exist.", file=sys.stderr) You'll need to import sys.
  • You may want to specify in your doc string the error messages that can be emitted.
  • I would question whether the function is named appropriately: it does open the file, but solely for the purpose of verifying that it can be opened. Perhaps something like prompt_for_and_verify_filename is more apt.
  • Nothing about this seems specific to CSV files.
  • You don't run any checks on the filename provided. Are you sure you trust your user not to input a path you don't want your program touching?
\$\endgroup\$
3
  • \$\begingroup\$ Thank you for the reply! How would you go about making the newlines look more appealing? I see the unnecessary f-string now, it's an f-string because there was a set of { } inside it, just forgot to remove the f. That's a good point regarding the docstring, I shall update that now, I shall also remove the break, good to know that the return will end the loop - I'll rectify the inconsistency with ERROR: - but why would I want error messages printed to standard error? I haven't come across that before. \$\endgroup\$ Commented Sep 14 at 22:38
  • 4
    \$\begingroup\$ @Jake, why would you want error messages mixed in with your standard output? If you're using the command in a pipeline, you don't normally want the errors to appear as input to the next command. In cases like this where the program hasn't performed its task, it's also a good idea to exit with a non-zero status so that other commands know it has failed. \$\endgroup\$ Commented Sep 15 at 6:56
  • \$\begingroup\$ To be fair I think my response regarding errors may have come across more aggressively than I intended - after reading the why, this makes a lot more sense - I'll keep this in mind and adjust - thank you. \$\endgroup\$ Commented Sep 16 at 4:14
8
\$\begingroup\$

I will not repeat what others have so far said.

Open the File Correctly!

@Chris commented that "Nothing about this seems specific to CSV files."

I would go one step further and tell you that the way you are opening files will not always work if its intended use of the file is with the csv module. If you look at the documentation for the csv module, specifically the footnote at the bottom of the page, you will see:

If newline='' is not specified, newlines embedded inside quoted fields will not be interpreted correctly, and on platforms that use \r\n line endings on write an extra \r will be added. It should always be safe to specify newline='', since the csv module does its own (universal) newline handling.

But this requirement is not specified only in a footnote. If you look at the description for csv.reader on this page you will see:

If csvfile is a file object, it should be opened with newline=''.

And the following example is provided:

import csv
with open('eggs.csv', newline='') as csvfile:
    spamreader = csv.reader(csvfile, delimiter=' ', quotechar='|')
    for row in spamreader:
        print(', '.join(row))

All csv functions that take as an argument an open file should ensure the file is opened specifying newline='' to work correctly for all platforms and CSV contents.

Remove Unnecessary Open Check

The latest documentation for the built-in open is a bit inconsistent. It states:

Open file and return a corresponding file object. If the file cannot be opened, an OSError is raised.

But later on in the documentation it states that as of release 3.3:

FileExistsError is now raised if the file opened in exclusive creation mode ('x') already exists.

And we know that is you are opening a file for reading and the file does not exist, you get a FileNotFoundError.

But in the end, if the file cannot be successfully opened, an exception is raised. Testing for file.closed is something you might want to do for a previously successfully opened file to see if it has been subsequently closed. So you can remove this check. Also, as of Python 3.3:

IOError used to be raised, it is now an alias of OSError.

So, assuming you want to return the open file, you might code the function as:

def open_csv_file():
    """
    Display the prompt for the user to specify the path for the CSV file that they wish to open in the program.
    
    Returns
        The opened CSV file
    """
    # Infinite loop to open the CSV file, if file not found then it will loop again until the file is found.
    while True:
        menu.print_menu_prompt()

        timesheet_csv = input("\nPlease specify the CSV path to be opened: ")

        # Try and except to check if the CSV file has opened successfully, if so then it is returned to main.
        try:
            file = open(timesheet_csv, 'r', newline='\n')
        except FileNotFoundError:
            print(f"\nERROR: The specified file does not exist.")
        except IOError:
            print("\nAn error occured while trying to open the CSV file.")
        else:
            print("\nThe CSV file opened successfully")
            return file

But how does the user escape this function if they cannot specify the path of a CSV file? See the next section!

Separation of Concerns

You should have one function that takes as an argument a path to a CSV file and whose sole concern is the opening of that file. The function either returns the opened file or raises an exception if the file cannot be opened. Then a second function would be concerned with prompting the user for the path. If the user specifies an empty string, you would "escape" this function. Otherwise, the path would be passed to the first function.

\$\endgroup\$
1
  • \$\begingroup\$ Yes I suppose this is more general to opening any time of files rather than a CSV, I had planned and have been working with pandas rather than CSV as I plan to manipulate the data within - I'll take a look at the documentation for the CSV module and go from there! Thanks for the help. \$\endgroup\$ Commented Sep 16 at 4:12
5
\$\begingroup\$

In addition to the good points made in the previous answer, here are a few suggestions.

Tools

You could run code development tools to automatically find some style issues with your code.

ruff and pylint identify the unneeded f-string in main.py

In this case, I recommend that you keep the f-string, and add the file name variable to it:

print(f"\nERROR: The specified file does not exist: {timesheet_csv}")

This provides the user helpful debug information. It positively confirms what the code thinks the file name is, which is especially handy when the user includes a directory path to the file.

pylint also identifies the unreachable break statement. These 2 points were raised in the previous answer; I am just demonstrating what the tools can do for you.

Main guard

You can add a "main" guard in the main.py file so that you can show an example of how the code is run. This would go after the function definition:

if __name__ == '__main__':
    print(open_file_prompt())

Typo

I ran a spell checker on the code, and this line has a typo ("occured"):

print("\nAn error occured while trying to open the CSV file.")

Here's the fix:

print("\nAn error occurred while trying to open the CSV file.")

Error message

You can print the exact error message when IOError occurs:

except IOError as e:
    print(f"\nAn error occurred while trying to open the CSV file: {e}.")

This will help the user to debug problems. For example, if the file does exist, but you don't have permissions to read it, you could get an error message like this:

An error occurred while trying to open the CSV file: [Errno 13] Permission denied: 'file.csv'.

\$\endgroup\$
4
  • \$\begingroup\$ I do have a main guard in main.py - took the advice from yesterday's question and added one to main when I wrote it - I'll make sure to run a spell checker on my string literals more often. Assuming the except IOError as e is to show the exact error message if one of those crashes occurs? If so that's a great idea and I'll adopt it going forward. I've never heard of ruff and pylint, I'll look into these tools as they seem incredibly useful. \$\endgroup\$ Commented Sep 15 at 1:44
  • \$\begingroup\$ @JakeIsaacHarvey: I updated the answer to show an actual error message for the IOError case. \$\endgroup\$ Commented Sep 15 at 10:31
  • \$\begingroup\$ @JakeIsaacHarvey: I updated the answer again to demonstrate using the file name variable in the case where the file is not found. \$\endgroup\$ Commented Sep 15 at 12:18
  • 1
    \$\begingroup\$ Yes, and please wrap that {timesheet_csv} format placeholder with extra quotes. It really helps to see if there's, for example, any extraneous leading/trailing whitespace, almost impossible to see without surrounding quotes. \$\endgroup\$ Commented Sep 16 at 17:01
3
\$\begingroup\$

A few observations:

  1. This is a very unfriendly way to select a file. Your user can't use even use traditional command line interface conveniences like DIR or navigating folder by folder: they need to know the complete filepath and enter it with no errors.

    You will save users (and yourself!) a lot of hassle by using something like tkinter.filedialog instead.

    This is 100% an easy win. You can quickly write (or google and adapt) a short snippet of code which will be endlessly useful.

  2. The user has no way to graceful way to quit out of the loop. This is probably unproblematic in your intended use case but it isn't a clean design.

  3. I don't understand why the print_menu_prompt() function is stored in a different file.

    The message contained in the function looks very specific to this use case. I can't imagine that you will call the function anywhere else in your code. Which begs the question: why is it in a different file? If you want to edit the message then it is helpful to be able to see the function which initiates it, and if you want to edit the function which initiates it then it will be helpful to see the error message. Separating one action into two different files feels like you're going out of your way to make maintenance annoying.

    I can see why you've broken it out into a separate function. Maybe it could be a sub-function? In this case I don't think you even need to do that: the prose is short enough that putting it into the while loop would have been fine.

No criticism intended: these are preferences and it takes time to explore what works and what doesn't.

\$\endgroup\$
7
  • \$\begingroup\$ Thanks for replying! I plan to add a dialog navigation to the file once the rest of the program is built out - I wasn't sure how to do that yet so was waiting until I learnt, I'll look into tkinter.filedialog! I see how having no graceful way to quit is unclean, I wasn't sure of how to rectify this though. I put the print_menu_prompt() in a different file for the sake of readability - I can always add it back, even as function as the bottom of the file! I appreciate the criticism, best way to learn! Thanks for the help. \$\endgroup\$ Commented Sep 16 at 4:18
  • 1
    \$\begingroup\$ Please don't suggest tkinter.filedialog (GUI) for a CLI app... There are TUI file pickers, but please do not assume that a program running in CLI will have access to any desktop environment and ability to open GUI elements, and even if it has - that the end user wants a UI. Just imagine find or grep slapping you in the face with a GUI dialog every time you mistype or omit the filename. Oh, wait, sounds like MS Windows... \$\endgroup\$ Commented Sep 16 at 17:04
  • 1
    \$\begingroup\$ @STerliakov do you have a recommendation? Forcing users to enter their own filepaths is a poor choice in the cases where a GUI is available (most cases). Writing a custom CLI file picker doesn't seem proportionate to the problem. Is there a widely used and ideally low-dependency library that solves the problem with a CLI? \$\endgroup\$ Commented Sep 16 at 23:19
  • \$\begingroup\$ @P.Hopkinson My "recommendation" is in the standard library. Takes ~3 lines to set up (or ~30 to write your custom completer that, for example, excludes "hidden" files unless asked with an explicit period): import readline; readline.parse_and_bind('tab: complete'); readline.set_completer_delimiters(' \n\t'). That's not a file picker, but anyone familiar with CLIs will immediately know how to tab-complete paths:) Nowadays should work on all platforms - there are Mac and Windows surrogates that try to resemble GNU readline, and python wraps the available one. \$\endgroup\$ Commented Sep 17 at 11:29
  • \$\begingroup\$ (and usually you won't need even that, if possible - better take file paths as sys.argv, users will be familiar with how their own shell behaves and won't have to adapt to your implementation at all) \$\endgroup\$ Commented Sep 17 at 11:33

You must log in to answer this question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.