classification
Title: ConfigParser .read() - handling of nonexistent files
Type: enhancement Stage: resolved
Components: Library (Lib) Versions: Python 3.9
process
Status: closed Resolution: rejected
Dependencies: Superseder:
Assigned To: Nosy List: Adrian Wielgosik, BTaskaya, dheiberg, hongweipeng, lukasz.langa, serhiy.storchaka, terry.reedy, vstinner
Priority: normal Keywords: patch

Created on 2018-12-09 16:35 by Adrian Wielgosik, last changed 2019-10-28 19:29 by terry.reedy. This issue is now closed.

Pull Requests
URL Status Linked Edit
PR 16920 closed BTaskaya, 2019-10-25 03:55
Messages (9)
msg331439 - (view) Author: Adrian Wielgosik (Adrian Wielgosik) * Date: 2018-12-09 16:35
Documentation of ConfigParser says:

> If a file named in filenames cannot be opened, that file will be ignored. This is designed so that you can specify an iterable of potential configuration file locations (for example, the current directory, the user’s home directory, and some system-wide directory), and all existing configuration files in the iterable will be read.

While this is a useful property, it can also be a footgun. The first read() example in the Quick Start section contains just a single file read:

>>> config.read('example.ini')

I would expect that this basic usage is very popular. If the file doesn't exist, the normal usage pattern fails in a confusing way:

 from configparser import ConfigParser
 config = ConfigParser()
 config.read('config.txt')
 value = config.getint('section', 'option')
---> configparser.NoSectionError: No section: 'section'

In my opinion, this error isn't very obvious to understand and debug, unless you have read that piece of .read() documentation.

This behavior did also bite me even more, with another usage pattern I've found in a project I maintain:
> config.read('global.txt')
> config.read('local.txt')
Here, both files are expected to exist, with the latter one extending or updating configuration from the first file. If one of the files doesn't exist (eg mistake during deployment), there's no obvious error, but the program will be configured in different way than intended.

Now, I'm aware that all of this can be avoided by simply using `read_file()`:
> with open('file.txt') as f:
>    config.read_file(f)
But again, `.read()` is the one usually mentioned first in both the official documentation, and most independent guides, so it's easy to get wrong.

Due to this, I propose adding an extra parameter to .read():
read(filenames, encoding=None, check_exist=False)
that, when manually set to True, will throw exception if any of input files doesn't exist; and to use this parameter by default in Quick Start section of ConfigParser documentation.
If this is a reasonable idea, I could try and make a PR.

For comparison, the `toml` Python library has the following behavior:
- if argument is a single filename and file doesn't exist, it throws
- if argument is a list of filenames and none exist, it throws
- if argument is a list of filenames and at least one exists, it works, but prints a warning for each nonexistent file.

For the record, seems like this issue was also mentioned in https://bugs.python.org/issue490399
msg331862 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2018-12-14 20:05
Since the code and doc agree, and since the proposal is to add a call parameter, this would be an enhancement for next release only, not a bug fix.

The proposal seems reasonable.  I might use it for IDLE.

IDLE uses .read within this subclass method.

    def Load(self):
        "Load the configuration file from disk."
        if self.file:  # '' for at least some tests
            self.read(self.file) 

The default config files in idlelib should be present.  (I should see what happens if not.  Does every 'get' pass a seeming redundant and possibly inconsistent backup default?)  I might use the new parameter here.  

User override config files and even the config directory are optional, so the current behavior is fine.
msg355135 - (view) Author: Batuhan (BTaskaya) * Date: 2019-10-22 15:20
Adrian are you still want to work on this or can i take it?
msg355156 - (view) Author: Adrian Wielgosik (Adrian Wielgosik) * Date: 2019-10-22 21:32
Yeah, I lost steam on this issue, sorry. Go ahead :)
msg355364 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2019-10-25 12:45
I do not think that adding an extra parameter to .read() will solve any problem.

You need to read the documentation of read() to use this feature. You need to change your code, and this will work only in new version of Python, so you will need to support two versions of the code in any way. If you change your code, it is easier to make it using read_file(), this way is compatible with old Python versions, and you can use it today.

If you do not read the documentation and do not change your code this feature cannot help you. Changing the default behavior will break existing code.
msg355371 - (view) Author: Batuhan (BTaskaya) * Date: 2019-10-25 19:05
> I do not think that adding an extra parameter to .read() will solve any problem.

There is a use case of this (which some of tools depends) about checking if configuration exists and if not, raising an error. Now, they can solve this by just adding check_exist argument. 

> Changing the default behavior will break existing code.

Can you give an example of how this feature can/could break existing code?
msg355557 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2019-10-28 16:04
> I would expect that this basic usage is very popular. If the file doesn't exist, the normal usage pattern fails in a confusing way: (...)

Well, the configparser is well defined. As you wrote: "If a file named in filenames cannot be opened, that file will be ignored."

You can check if the file exists or not by checking read() result:

"read(): (...) returning a list of filenames which were successfully parsed."
https://docs.python.org/dev/library/configparser.html#configparser.ConfigParser.read

I suggest to close this issue as "not a bug".

--

For me, a better option would be to be able to pass an open file to configparser. So the caller can decide how to handle the open() error.
msg355560 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2019-10-28 16:17
> There is a use case of this (which some of tools depends) about checking if configuration exists and if not, raising an error. Now, they can solve this by just adding check_exist argument.

No, it can be solved by using open() and read_file(). It can also be solved by checking the result of read(), as Victor suggested. Your proposition adds third way, but unlike to the first two it could be used only in new Python versions.

> Can you give an example of how this feature can/could break existing code?

You pass a list containing user configuration path, system-wide configuration path, and default configuration path (it can contain also per-directory configuration path). At first run of your program there is no user configuration file.

> For me, a better option would be to be able to pass an open file to configparser. So the caller can decide how to handle the open() error.

There is the read_file() method which accept an open file.
msg355590 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2019-10-28 19:29
FWIW, I agree with closing this.  I already changed my mind from my earlier comment and decided that IDLE should maybe switch to using open and read_file in different places, for the reasons given above.  It might make testing without depending on a local config, which does not exist on buildbots, easier.
History
Date User Action Args
2019-10-28 19:29:55terry.reedysetmessages: + msg355590
2019-10-28 16:17:21serhiy.storchakasetstatus: open -> closed
resolution: rejected
messages: + msg355560

stage: patch review -> resolved
2019-10-28 16:04:27vstinnersetnosy: + vstinner
messages: + msg355557
2019-10-25 19:05:42BTaskayasetmessages: + msg355371
2019-10-25 12:45:24serhiy.storchakasetnosy: + serhiy.storchaka
messages: + msg355364
2019-10-25 03:55:11BTaskayasetversions: + Python 3.9, - Python 3.8
2019-10-25 03:55:00BTaskayasetkeywords: + patch
stage: test needed -> patch review
pull_requests: + pull_request16452
2019-10-22 21:32:03Adrian Wielgosiksetmessages: + msg355156
2019-10-22 15:20:48BTaskayasetnosy: + BTaskaya
messages: + msg355135
2018-12-14 20:05:28terry.reedysetversions: + Python 3.8
nosy: + terry.reedy

messages: + msg331862

type: behavior -> enhancement
stage: test needed
2018-12-11 06:40:08hongweipengsetnosy: + hongweipeng
2018-12-10 20:28:32dheibergsetnosy: + dheiberg
2018-12-09 18:02:27xtreaksetnosy: + lukasz.langa
2018-12-09 16:35:30Adrian Wielgosikcreate