bpo-33504: Migrate configparser from OrderedDict to dict. #6819
Conversation
|
Hello, and thanks for your contribution! I'm a bot set up to make sure that the project can legally accept your contribution by verifying you have signed the PSF contributor agreement (CLA). Unfortunately our records indicate you have not signed the CLA. For legal reasons we need you to sign this before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue. When your account is ready, please add a comment in this pull request Thanks again to your contribution and we look forward to looking at it! |
Lib/configparser.py
Outdated
| @@ -157,6 +157,7 @@ | |||
| "LegacyInterpolation", "SectionProxy", "ConverterMapping", | |||
| "DEFAULTSECT", "MAX_INTERPOLATION_DEPTH"] | |||
|
|
|||
| DEFAULT_DICT = dict | |||
There was a problem hiding this comment.
What is the purpose of introducing this variable?
There was a problem hiding this comment.
The unit tests reach into configparser to use the default dict implementation, and I was unsure of whether it would be best practice to continue that behavior or just make the unit tests reference dict directly.
There was a problem hiding this comment.
Rename this back to _default_dict to keep the previous API.
|
Updated to retain |
|
Added news entry. |
| @@ -445,7 +445,7 @@ the :meth:`__init__` options: | |||
| Hint: if you want to specify default values for a specific section, use | |||
| :meth:`read_dict` before you read the actual file. | |||
|
|
|||
| * *dict_type*, default value: :class:`collections.OrderedDict` | |||
| * *dict_type*, default value: :class:`dict` | |||
There was a problem hiding this comment.
It would be nice to reword the rest of this section. It suggests below that "you can also use a regular dictionary" (but we already are!) and that "When you use a regular dictionary in those operations, the order of the keys may be random." which is no longer true.
Would be good to change this to describe what is actually happening
There was a problem hiding this comment.
Adding .. versionchanged:: would be also nice.
|
I don't think that the versionchanged directive and a news entry are needed if this type is not leaked to a user. It is just an implementation detail. The order of items is preserved, and it doesn't matter what data structure is used for satisfying this. |
With 3.7+, dictionary are ordered by design. Configparser still uses collections.OrderedDict, which is unnecessary. This updates the module to use the standard dict implementation by default, and changes the docs and tests to match.
|
Updated documentation. |
|
Is there anything else I need to do, or is this ready to be merged? |
|
@ambv: Please replace |
With 3.7+, dictionary are ordered by design. Configparser still uses
collections.OrderedDict, which is unnecessary. This updates the module
to use the standard dict implementation by default, and changes the
docs and tests to match.
https://bugs.python.org/issue33504