gh-127011: Add __str__ and __repr__ to ConfigParser#127014
gh-127011: Add __str__ and __repr__ to ConfigParser#127014Agent-Hellboy wants to merge 36 commits intopython:mainfrom
Conversation
ZeroIntensity
left a comment
ZeroIntensity
left a comment
There was a problem hiding this comment.
Please add a NEWS entry and tests.
Lib/configparser.py
Outdated
|
|
||
| def __str__(self): | ||
| config_dict = { | ||
| section: dict(self.items(section)) for section in self.sections() |
There was a problem hiding this comment.
This performs interpolations on all of the values, which can raise InterpolationError (or other arbitrary exceptions, depending on what interpolation class was specified). I think generally __str__ should be "safe" and not perform operations that can fail unexpectedly
There was a problem hiding this comment.
okay, I have ensured that str provides a raw representation of the configuration, avoiding interpolation to prevent unexpected errors.
jaraco
left a comment
jaraco
left a comment
There was a problem hiding this comment.
This is looking great. Can you add some examples in the description of the PR that illustrate what the output looks like for str and repr in a few representative scenarios?
|
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated. Once you have made the requested changes, please leave a comment on this pull request containing the phrase |
|
I have made the requested changes; please review again |
|
Thanks for making the requested changes! @jaraco: please review the changes made to this pull request. |
Lib/configparser.py
Outdated
| def __str__(self): | ||
| config_dict = { | ||
| section: {key: value for key, value in self.items(section, raw=True)} | ||
| for section in self.sections() | ||
| } | ||
| return str(config_dict) |
There was a problem hiding this comment.
I think this implementation leaves too much risk for confusion. With this, you can have str(config) render as "{'a': {'b': XYZ}}", but have config["a"]["b"] evaluate to something other than XYZ (or even raise an exception). That seems rather undesirable, especially for something that's intended to be used for debugging/logging.
Personally, I don't see the benefit in defining __str__. I think it would be better to let uses pick the dict-like representation they want and the tradeoffs to go with. For example, users that want an exception-safe dict representation without interpolations can get one with ConfigParser._sections, and users that a want dict representation that matches the runtime indexing behavior (but which may raise exceptions) can use {k: dict(v) for k, v in config.items()}.
There was a problem hiding this comment.
Hi @brianschubert
Thank you for the feedback. Could you please share an example to help clarify your concerns?
Based on your input, I plan to make it fallback to repr for string representations and add a new API, to_dict, which could be useful for scenarios like dumping the configuration into JSON and several other use cases.
I’m waiting for others to agree with you before proceeding.
There was a problem hiding this comment.
Leave a new API to another PR, let's just focus on __str__ and __repr__ here. Regarding what __str__ should do, I suggested in the issue that it display <ConfigParser: {json representation ...}> to mitigate confusion with dictionaries. I think that (or a variation of it) is the best approach here.
IMO, recommending _sections is a terrible idea for debugging, and we definitely should not write off a feature because it exists. Namely:
- It's undocumented, so users have no idea what it will do.
- Futhermore to being undocumented, it's not exposed by typeshed.
- And most importantly, it's a private implementation detail. Users aren't supposed to rely on it, because we're allowed to change (or remove) it at any time in patch releases.
There was a problem hiding this comment.
And most importantly, it's a private implementation detail. Users aren't supposed to rely on it, because we're allowed to change (or remove) it at any time in patch releases.
exactly.
Leave a new API to another PR, let's just focus on
__str__and__repr__here. Regarding what__str__should do, I suggested in the issue that it display<ConfigParser: {json representation ...}>
Done
There was a problem hiding this comment.
Hi @ZeroIntensity , should I create an issue for adding to_dict ?
jaraco
left a comment
jaraco
left a comment
There was a problem hiding this comment.
LGTM, modulo a resolution on how to handle __str__.
Agent-Hellboy
requested review from
1st1,
Fidget-Spinner,
ambv,
asvetlov,
brandtbucher,
carljm,
ezio-melotti,
iritkatriel,
methane,
ncoghlan,
picnixz,
savannahostrowski,
serhiy-storchaka,
tiran,
vsajip and
warsaw
as code owners
|
Hi @jaraco , This looks good to merge. We had a consensus pending the decision on str, which has been addressed. Apologies for the delay in following up. |
|
I'm going to close this. An incorrect rebase managed to tag every codeowner, and removing the request for review will not unsubscribe them, so any more activity here will flood inboxes. Please open a new PR. |
|
ohh sorry @ZeroIntensity , it was approved actually. created #132966 |
5 participants
Uh oh!
There was an error while loading. Please reload this page.