bpo-32360: Replace OrderedDict with dict in inspect.py module#5000
bpo-32360: Replace OrderedDict with dict in inspect.py module#5000srinivasreddy wants to merge 2 commits intopython:masterfrom srinivasreddy:32360_inspect
Conversation
srinivasreddy
changed the title
Lib/inspect.py
Outdated
There was a problem hiding this comment.
Should be: dict
(Docs don’t use English capitalization rules for code references like class names)
|
@merwok please take a look. |
|
@rhettinger ping. |
Lib/inspect.py
Outdated
| params = OrderedDict(((param.name, param) | ||
| for param in parameters)) | ||
| params = dict(((param.name, param) | ||
| for param in parameters)) |
There was a problem hiding this comment.
Minor: needless extra set of parens here.
params = dict((param.name, param) for param in parameters)
# or simply:
params = {param.name: param for param in parameters}
| @@ -0,0 +1 @@ | |||
| Remove references of OrderedDict in the module `inspect` | |||
There was a problem hiding this comment.
IMO «reference» is vague and «import» would be clearer here.
Lib/inspect.py
Outdated
| import builtins | ||
| from operator import attrgetter | ||
| from collections import namedtuple, OrderedDict | ||
| from collections import namedtuple |
There was a problem hiding this comment.
If inspect needs to import collections anyway, I don’t think there is much value in removing OrderedDict.
Also replacing params.move_to_end(p) by params[p] = params.pop(p) without even a comment makes the code a bit worse to read.
I would be -1 on this PR.
(I thought I already left this comment? Are there duplicate PRs?)
|
@merwok Done. |
|
I did not mean to ask to remove one line in your patch (the import OrderedDict, which in your latest version is an unused import). I meant that the whole PR seems in the end not needed, since we end up importing collections anyway, and using OrderedDict makes for clearer code. |
4 participants
https://bugs.python.org/issue32360