gh-141510: Use frozendict in the stdlib#144909
Conversation
Co-authored-by: Donghee Na <donghee.na@python.org>
|
This PR only changes private names, so it should not impact stdlib users. |
|
If this change is merged, I will propose a follow-up PR which modify public names. |
AlexWaygood
left a comment
There was a problem hiding this comment.
One optional suggestion in typing.py
picnixz
left a comment
There was a problem hiding this comment.
Ok for ssl and a suggestion for opcode
Lib/opcode.py
Outdated
| _cache_format = { | ||
| "LOAD_GLOBAL": { | ||
| _cache_format = frozendict( | ||
| LOAD_GLOBAL={ |
There was a problem hiding this comment.
The values of LOAD_GLOBAL can also be a frozendict (that is, we can have nested frozendicts).
|
The SC does not allow mechanically replacing all uses of dict. Please wait for reviews from the code owners of each module. |
|
What's the benefit of changing this in e.g. typing.py? It might break some users who modify this dict. That's not a supported use case, but we shouldn't go out of our way to break users unless there's some benefit. |
The purpose of this change is to prevent dictionary modifications, especially if the modification is done by mistake. |
|
@JelleZijlstra: Do you prefer that I revert the |
|
While the variables in stringprep are public, note that the file is autogenerated: # This file is generated by mkstringprep.py. DO NOT EDIT.And the docs clearly state:
So, I really think we should consider any direct use or mutation of those variables as really a bad idea and I don't think we should even consider code that messes with that as being legitimate. |
I revert the stringprep change to restrict this change to private variables. Changing private variables type should be less controversial than changing public variables type. |
|
I reverted the |
|
If someone really want/need to modify one of the frozendict, it remains possible. Example: $ ./python
>>> import platform
>>> platform._ver_stages
frozendict({'dev': 10, 'alpha': 20, 'a': 20, 'beta': 30, ..., 'p': 200})
>>> platform._ver_stages['final']=60
...
TypeError: 'frozendict' object does not support item assignment
>>> platform._ver_stages |= frozendict(final=60)
>>> platform._ver_stages
frozendict({'dev': 10, 'alpha': 20, 'a': 20, 'beta': 30, ..., 'p': 200, 'final': 60})The purpose of this change is more to catch dictionaries modified by accident. |
|
I merged my PR, thanks for reviews. If someone comes up with an concrete use case to modify one of these private attributes, we can revert the affected attribute. |
Uh oh!
There was an error while loading. Please reload this page.