Skip to content
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

Add __str__ and __repr__ to ConfigParser for Better Debugging and Unit Testing #127011

Open
Agent-Hellboy opened this issue Nov 19, 2024 · 12 comments
Labels
stdlib Python modules in the Lib dir type-feature A feature request or enhancement

Comments

@Agent-Hellboy
Copy link
Contributor

Agent-Hellboy commented Nov 19, 2024

Feature or enhancement

Proposal:

The ConfigParser class supports dictionary-like operations, including the contains protocol for checking section existence using the in operator. However, it lacks str and repr methods, essential for providing human-readable and developer-friendly representations of the configuration data. Adding these methods would enhance its usability, particularly for debugging, logging, and unit testing, by offering clear insights into its internal state.

current behavior:

str_resp: <configparser.ConfigParser object at 0x7f57e7cc5160>
repr_resp: <configparser.ConfigParser object at 0x7f57e7cc5160>

suggested behavior:

str_resp: {'GERRIT': {'api_key': 'val'}}
repr_resp: <ConfigParser: {'GERRIT': {'api_key': 'val'}}>

I haven’t started a thread on Python Discourse as this is a straightforward feature request requiring a simple accept or reject decision. I'd appreciate the clarification on whether there was a specific design reason for not including str and repr. If approved, I will raise a PR.

Has this already been discussed elsewhere?

This is a minor feature, which does not need previous discussion elsewhere

Links to the previous discussion of this feature:

No response

Linked PRs

@Agent-Hellboy Agent-Hellboy added the type-feature A feature request or enhancement label Nov 19, 2024
@ZeroIntensity
Copy link
Member

I think it makes sense to add a __str__ and __repr__, but I'm not so sure about the proposed design. I'm fine with the proposed __str__ (it's fine for users to confuse a ConfigParser for a dictionary because it's dict-like), but the __repr__ might be problematic. Generally, the rule of thumb is that a __repr__ should return an expression that creates the same object, so it should really display the documented initargs here rather than the contents (but there are exceptions to this rule all the time).

Go ahead and send a PR, and we can decide whether or not to go with it from there :)

@ZeroIntensity ZeroIntensity added the stdlib Python modules in the Lib dir label Nov 19, 2024
Agent-Hellboy added a commit to Agent-Hellboy/cpython that referenced this issue Nov 19, 2024
@Agent-Hellboy
Copy link
Contributor Author

the rule of thumb is that a repr should return an expression that creates the same object, so it should really display the documented initargs here rather than the contents

I have used default values

@terryjreedy
Copy link
Member

The main issue to me is that a ConfigParser can have 100s of entries. Of course, the same is true of dicts ans other containers. And unittest special cases susch builtins to condense error messages.

@Agent-Hellboy
Copy link
Contributor Author

The main issue to me is that a ConfigParser can have 100s of entries. Of course, the same is true of dicts ans other containers. And unittest special cases susch builtins to condense error messages.

Why not leave it as is? Users who want to print the output are likely dealing with small configuration files anyway. Alternatively, if necessary, I could truncate the dictionary representation with ... for larger files.

@brianschubert
Copy link
Contributor

I don't think ConfigParser should be rendered exactly like a dict. That seems like too high a risk for confusion, especially since ConfigParser isn't fully compatible with dict/Mapping:

import configparser

config = configparser.ConfigParser()
config.read_string("""
[foo]
a = 1
""")

>>> config.get("foo")
Traceback (most recent call last):
  File "<python-input-21>", line 1, in <module>
    config.get("foo")
    ~~~~~~~~~~^^^^^^^
TypeError: RawConfigParser.get() missing 1 required positional argument: 'option'

If you what you want is a convenient shorthand for debugging, then ConfigParser._sections may suit your needs better than a __str__/__repr__ implementation:

>>> config._sections
{'foo': {'a': '1'}}

This gives you essentially the same result as the proposed __str__ implementation, just without running the interpolations.

@Agent-Hellboy
Copy link
Contributor Author

Hi @brianschubert

The example shared highlights a difference between ConfigParser and dict, but it doesn’t invalidate the idea of enhancing ConfigParser for better debugging. I’m not suggesting that ConfigParser should fully mimic a dict—if that were the goal, it would have been inherited from Mapping. The comparison to dict was to emphasize usability and debugging convenience, not to imply complete parity.

Relying on private APIs like _sections for debugging isn’t ideal. While people can and do use private attributes occasionally, providing a Pythonic approach like improving str or repr would offer a clean, public way to inspect configurations. This doesn’t conflict with the existing API but complements it, enhancing usability for developers without changing its core behavior.

@jaraco
Copy link
Member

jaraco commented Nov 22, 2024

I feel like if the parameters are included in the __repr__, there should be some methodology for deciding which parameters should be included and which not. The proposed approach only shows two seemingly arbitrarily-selected parameters of the 12 the constructor accepts. My preference would be to show all of the parameters, but only those that the caller supplied when constructing the parser. Of course, that would be more involved, especially if you want to detect if the caller explicitly supplied a default value (I don't think you need to do that).

Unfortunately, the most important summary information about the state isn't part of the constructor. For example, there's no indication if the configparser has loaded a file or files (or which ones), or how many sections or keys it might have. That information might be more useful to the user. You may also want to consider providing other state, like (null) (for prior to binding a file stream) or (13 sections).

@Agent-Hellboy
Copy link
Contributor Author

Hi @jaraco Thanks for the feedback , I have added a new commit

what do you think of

repr: <ConfigParser(params={'defaults': {'compression': 'yes', 'server_alive_interval': '45', 'compression_level': '9'}, 'dict_type': 'type', 'allow_no_value': False, 'delimiters': ('=', ':'), 'strict': True, 'default_section': 'DEFAULT', 'interpolation': 'BasicInterpolation'}, state={'loaded_files': ['example.ini'], 'sections': 2})>
str: {'User': {'compression': 'yes', 'server_alive_interval': '45', 'compression_level': '9', 'name': 'John Doe', 'email': 'john.doe@example.com'}, 'Database': {'compression': 'yes', 'server_alive_interval': '45', 'compression_level': '9', 'host': 'localhost', 'port': '5432', 'username': 'admin', 'password': 'secret'}}

@ZeroIntensity
Copy link
Member

IMO, this kind of style you suggested for the repr would work fine for the str:

<ConfigParser: {'GERRIT': {'api_key': 'val'}}>

It doesn't break any conventions since it's __repr__, clearly shows the content, and doesn't get it confused with a dictionary.

@Agent-Hellboy
Copy link
Contributor Author

It doesn't break any conventions since it's __repr__, clearly shows the content, and doesn't get it confused with a dictionary.

Kind of, but from the Python docs and this StackOverflow post, it says repr should be used to get the object's representation, and str is for the end user. I guess people like us are going to use str only.

@ZeroIntensity
Copy link
Member

Yes, that follows that, doesn't it? Users will see the __str__ when printing and whatnot, and then the repr otherwise.

@Agent-Hellboy
Copy link
Contributor Author

Yes, I picked you only after your first comment, but after Jaraco’s comments, I thought repr should give every piece of information (object state) possible about the object, and str a dict representation of the object. We can use the parser object as a dict, so we will give a dict to complement its behavior.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stdlib Python modules in the Lib dir type-feature A feature request or enhancement
Projects
None yet
Development

No branches or pull requests

5 participants