Skip to content

Unhelpful error message from logging.config.FileConfig can be improved #103606

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

Closed
vsajip opened this issue Apr 18, 2023 · 11 comments
Closed

Unhelpful error message from logging.config.FileConfig can be improved #103606

vsajip opened this issue Apr 18, 2023 · 11 comments
Assignees
Labels
3.11 only security fixes 3.12 only security fixes type-feature A feature request or enhancement

Comments

@vsajip
Copy link
Member

vsajip commented Apr 18, 2023

Feature or enhancement

If a configuration file does not exist, the error raised by logging.config.fileConfig doesn't say that the file doesn't exist, instead suggesting that the file has an invalid format. This should be improved so that the correct error message is provided.

Pitch

The improved error message will guide a user to an error such as a typo in the filename more quickly. See for example this question.

Linked PRs

@vsajip vsajip added the type-feature A feature request or enhancement label Apr 18, 2023
@vsajip vsajip self-assigned this Apr 18, 2023
@vsajip vsajip moved this to Todo in Logging issues 🪵 Apr 18, 2023
@Agent-Hellboy
Copy link
Contributor

Hi, @vsajip can I try to fix this?

@arhadthedev
Copy link
Member

@Agent-Hellboy It's an open project, no preliminary assignment is required. The only thing you need is to follow the contributor's manual at https://devguide.python.org/ where conventions and processes are described.

@Agent-Hellboy
Copy link
Contributor

okay, sure

@Agent-Hellboy
Copy link
Contributor

Agent-Hellboy commented Apr 19, 2023

Hi @arhadthedev
my approach would be to check the return value of cp.read(fname, encoding=encoding)

cp.read(fname, encoding=encoding)

It uses configparser to parse the configuration files which suppresses the OSError but it returns an empty list if the file is not present
def read(self, filenames, encoding=None):

I will raise a PR which will raise ValueError that the file is not present and will try to include a test for the same

@vsajip
Copy link
Member Author

vsajip commented Apr 20, 2023

Wouldn't it be simpler to use e.g. os.path.exists() and then throw a FileNotFoundError if the file doesn't exist? That would be the approach I would have taken.

@arhadthedev
Copy link
Member

Because of the easier to ask for forgiveness than permission principle with the very os.path.exists() as an example:

import os

# violates EAFP coding style
if os.path.exists("file.txt"):
    os.unlink("file.txt")

@vsajip
Copy link
Member Author

vsajip commented Apr 20, 2023

Yes, but the "file not found" message/FileNotFound exception are more informative than a generic "invalid file" when the file doesn't exist. It's a small point, I suppose; it's easier to ask for forgiveness, but that doesn't mean it's a cast-iron rule.

@Agent-Hellboy
Copy link
Contributor

okay, I will raise FileNotFound before giving the file to the config parser

if isinstance(fname, configparser.RawConfigParser):

why should I try to parse a file which is not present

@gpshead
Copy link
Member

gpshead commented May 20, 2023

I left some comments on the merged PR... ValueError isn't the right exception to use for things external to the process not related to the variable value itself. I suggest RuntimeError if you don't have your own Error exception class.

@Agent-Hellboy
Copy link
Contributor

sure @gpshead, I will raise a PR

Agent-Hellboy added a commit to Agent-Hellboy/cpython that referenced this issue May 20, 2023
gpshead pushed a commit that referenced this issue May 20, 2023
…4701)

(this adjusts new code) raise RuntimeError if provided config file is invalid or empty, not ValueError.
gpshead pushed a commit that referenced this issue May 21, 2023
GH-103628) (#104687)

* gh-103606: Improve error message from logging.config.FileConfig (GH-103628)

(cherry picked from commit 152227b)

plus backport the followup exception change fix to that in #104701
@gpshead
Copy link
Member

gpshead commented May 21, 2023

thanks!

@gpshead gpshead closed this as completed May 21, 2023
@github-project-automation github-project-automation bot moved this from Todo to Done in Logging issues 🪵 May 21, 2023
@gpshead gpshead added 3.11 only security fixes 3.12 only security fixes labels May 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3.11 only security fixes 3.12 only security fixes type-feature A feature request or enhancement
Projects
Development

No branches or pull requests

4 participants