Handle ValueError while reading configuration files by configparser
#122454
Comments
|
The body of the bug description doesn't describe the bug. I had to read through the diff of the PR to understand what the problem is. Can you describe what behavior you encountered under what conditions and what behavior you expected instead? |
|
Oups, sorry! I'm actually trying to fix the different calls to I personally did not use the module so I was just looking which modules were missing possible ValueError. I think that the description of
and this is done by suppressing OSError exceptions (in general, that's the error you have when you cannot open a file). Nevertheless, you can also have a ValueError when the filename has embedded NUL bytes, surrogate codes or if the name is just too long, and those files should be considered as "cannot be opened". |
|
Thanks for the explanation. Now that I think about it, I'm more inclined to think configparser shouldn't be changed. As you point out, the user can supply invalid filenames, but I wouldn't consider that "files that cannot be opened" in the same way that a missing file or inaccessible file cannot be opened. That is, an invalid filename is not valid for a "potential configuration file location", because it will never be a viable configuration file location. Propagating the ValueError in this case is just fine, and preferable, IMO. Can you think of a scenario where a user would be supplying an invalid filename and would want that error suppressed? |
|
|
Actually, let me just check with the CI/CD. I'll try to see what happens. |
I confirmed on macOS, a long filename also results in an OSError. On my Windows system, I get an On windows, I get FileNotFoundError for the surrogate escapes. That sounds not like a problem with configparser but a problem with But short of that, if the user is supplying an invalid filename for any given platform, it seems that the preferable behavior would be to raise an exception. I still don't think I have an answer to the question - do we want configparser to treat invalid filenames as valid but missing? Are there cases where, for example, a user is specifying a very long filename, and that name will be valid on one platform, but not another, and we want that code to fail silently on the platforms where it's invalid? I'm thinking no. In that case, the caller should be adding that protection. In the case of configparser, the error suppression is there for missing or inaccessible files and not invalid ones. |
In #122353 (comment), it was suggested to protect against It remains to decide how to handle invalid file names in general, but I think it's better to have a different issue (I'll close the PR as well, sorry for the time I took!) |
|
Er... actually... For long names, the behaviour is the same on Linux and Windows (namely an OSError). |
|
I'm still of the opinion that the current behavior is perfectly acceptable, that silently suppressing OSError and not suppressing ValueError is the best design for configparser and that it's the caller's responsibility to supply valid values for filenames. It seems to me we're not aware of a single instance where the current behavior fails to meet a user's expectations. I think it makes sense to explore the possibility that " My recommendation, if you feel otherwise, is to either provide a compelling case that would shift my position on the matter, or rally support from another core dev to your understanding. |
Yes, after a nmore careful thinking, I also think it's the best course. I incorrectly assumed that Closing as |
|
Thanks picnixz for the careful consideration. I know it's not fun to put a lot of effort into a contribution only not to make any change, so thanks for being cordial and amenable. |
Well, I work in academia: sometimes papers got accepted, sometimes not... So I'm not really bothered by the fact that I needed to put effort into something that is not accepted and I also understand the reasons (in this specific case, I personally feel that I was too hasty in making the change so it's not your fault at all!). I think I also learned things by doing this so it was beneficial (and thank you for pointing out the issues) |
Uh oh!
There was an error while loading. Please reload this page.
Bug report
Bug description:
In
configparser.RawConfigParser.read, whenopen()raisesOSError, the file is skipped. Note that theOSErrorguard also includes the parsing phase and not just the call toopen().However,
open()can raise a ValueError in the following cases:Note that there is a test for checking when a file cannot be read assuming it has been opened. In particular, to retain compatibility, we should only skip the file if we cannot open it in the first place. Any issue during the parsing (such as an encoding or decoding error) should be propagated (but an OSError during the parsing should be suppressed as it is currently the case).CPython versions tested on:
CPython main branch
Operating systems tested on:
No response
Linked PRs
ValueErrorinconfigparser.RawConfigParser.read[not ready for review] #122455The text was updated successfully, but these errors were encountered: