gh-127011: Add __str__ and __repr__ to ConfigParser #127014
Conversation
Agent-Hellboy
commented
Nov 19, 2024
•
Agent-Hellboy
commented
- Issue: Add __str__ and __repr__ to ConfigParser for Better Debugging and Unit Testing #127011
Lib/configparser.py
Outdated
| @@ -1040,6 +1042,33 @@ def __iter__(self): | |||
| # XXX does it break when underlying container state changed? | |||
| return itertools.chain((self.default_section,), self._sections.keys()) | |||
|
|
|||
| 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.
|
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 ?
