Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in

#16613: ChainMap.new_child could use improvement

Can't Edit
Can't Publish+Mail
Start Review
6 years, 5 months ago by vinay_sajip
6 years, 5 months ago
doerwalter, rhettinger, Vinay Sajip, r.david.murray, devnull_psf.upfronthosting.co.za

Patch Set 1 #

Total comments: 8
Unified diffs Side-by-side diffs Delta from patch set Stats Patch
Doc/library/collections.rst View 1 chunk +6 lines, -4 lines 2 comments Download
Lib/collections/__init__.py View 1 chunk +8 lines, -3 lines 0 comments Download
Lib/test/test_collections.py View 1 chunk +32 lines, -0 lines 6 comments Download


Total messages: 2
http://bugs.python.org/review/16613/diff/7089/Doc/library/collections.rst File Doc/library/collections.rst (right): http://bugs.python.org/review/16613/diff/7089/Doc/library/collections.rst#newcode87 Doc/library/collections.rst:87: of the parent mappings. This needs a versionchanged. http://bugs.python.org/review/16613/diff/7089/Lib/test/test_collections.py ...
6 years, 5 months ago #1
Vinay Sajip
6 years, 5 months ago #2
Thanks for the prompt review. Here are my responses to your comments.

File Doc/library/collections.rst (right):

Doc/library/collections.rst:87: of the parent mappings.
On 2013/01/11 13:47:42, ezio.melotti wrote:
> This needs a versionchanged.


File Lib/test/test_collections.py (right):

Lib/test/test_collections.py:122: self.assertEqual(d.maps, [{'b':20, 'c':30},
{'a':1, 'b':2}])  # check internal state
On 2013/01/11 13:47:42, ezio.melotti wrote:
> I would move the comment on the previous line, and avoid long lines.

I have kept it consistent with other tests, e.g. test_basic, since that seemed
more important than avoiding long lines.

Lib/test/test_collections.py:144: for k, v in dict(a=1, B=20, C=30,
z=100).items():             # check get
On 2013/01/11 13:47:42, ezio.melotti wrote:
> Ditto regarding these comments.
Again, if you see the other tests, you'll see the formatting of my addition is
consistent with those.

Lib/test/test_collections.py:145: self.assertEqual(d.get(k, 100), v)
On 2013/01/11 13:47:42, ezio.melotti wrote:
> Maybe there should be some test that checks what happen when a non-map is
> to new_child().

At the moment there is no check for a mapping type - should there be one? There
isn't anywhere else, e.g. the maps passed to the ChainMap are not tested to see
whether or not they are maps. I would say, there is no need for such a test in
just this case, since it wasn't judged necessary to have such tests elsewhere
for ChainMap.
Sign in to reply to this message.

RSS Feeds Recent Issues | This issue
This is Rietveld 894c83f36cb7+