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

Issue 4185044: Improve dbm modules — Python #9523

Can't Edit
Can't Publish+Mail
Start Review
Created:
9 years, 9 months ago by merwok
Modified:
9 years, 8 months ago
Reviewers:
ray
Base URL:
http://svn.python.org/view/*checkout*/python/branches/py3k/
Visibility:
Public.

Patch Set 1 #

Total comments: 26

Patch Set 2 : Updated versions #

Total comments: 52

Patch Set 3 : Updated version; another one will follow soon #

Total comments: 6

Patch Set 4 :   #

Unified diffs Side-by-side diffs Delta from patch set Stats (+2564 lines, -154 lines) Patch
Doc/library/dbm.rst View 1 2 3 3 chunks +9 lines, -6 lines 0 comments Download
Lib/dbm/__init__.py View 1 2 3 2 chunks +15 lines, -0 lines 0 comments Download
Lib/dbm/dumb.py View 1 2 3 1 chunk +0 lines, -6 lines 0 comments Download
Lib/dbm/gnu.py View 1 2 3 1 chunk +3 lines, -0 lines 0 comments Download
Lib/dbm/ndbm.py View 1 2 3 1 chunk +3 lines, -0 lines 0 comments Download
Lib/test/test_dbm.py View 1 2 3 3 chunks +3 lines, -3 lines 0 comments Download
Lib/test/test_dbm_base.py View 1 2 3 1 chunk +291 lines, -0 lines 0 comments Download
Lib/test/test_dbm_dumb.py View 1 2 3 3 chunks +28 lines, -1 line 0 comments Download
Lib/test/test_dbm_gnu.py View 1 2 3 1 chunk +37 lines, -57 lines 0 comments Download
Lib/test/test_dbm_ndbm.py View 1 2 3 1 chunk +11 lines, -25 lines 0 comments Download
Modules/_dbmmodule.c View 1 2 3 5 chunks +1026 lines, -3 lines 0 comments Download
Modules/_gdbmmodule.c View 1 2 3 6 chunks +1138 lines, -53 lines 0 comments Download

Messages

Total messages: 15
merwok
9 years, 9 months ago (2011-02-10 21:21:04 UTC) #1
merwok
http://codereview.appspot.com/4185044/diff/1/Doc/library/dbm.rst File Doc/library/dbm.rst (right): http://codereview.appspot.com/4185044/diff/1/Doc/library/dbm.rst#newcode72 Doc/library/dbm.rst:72: database modules. 3.2 being in rc stage, your additions ...
9 years, 9 months ago (2011-02-10 21:53:48 UTC) #2
ray
http://codereview.appspot.com/4185044/diff/1/Doc/library/dbm.rst File Doc/library/dbm.rst (right): http://codereview.appspot.com/4185044/diff/1/Doc/library/dbm.rst#newcode72 Doc/library/dbm.rst:72: database modules. On 2011/02/10 21:53:49, merwok wrote: > 3.2 ...
9 years, 9 months ago (2011-02-12 16:40:06 UTC) #3
merwok
This version of the patch looks even better! I have no idea about reference counting ...
9 years, 9 months ago (2011-02-12 20:14:50 UTC) #4
ray
http://codereview.appspot.com/4185044/diff/7002/Doc/library/dbm.rst File Doc/library/dbm.rst (right): http://codereview.appspot.com/4185044/diff/7002/Doc/library/dbm.rst#newcode72 Doc/library/dbm.rst:72: database modules. On 2011/02/12 20:14:50, merwok wrote: > Please ...
9 years, 9 months ago (2011-02-16 14:44:14 UTC) #5
merwok
I replied without quoting text, you can go to the codereview page to read my ...
9 years, 9 months ago (2011-02-16 16:34:05 UTC) #6
ray
Thanks again for such comments in details. And please forgive my sometimes English misunderstanding and ...
9 years, 9 months ago (2011-02-17 15:04:45 UTC) #7
merwok
> Thanks again for such comments in details. Thank you for responding to my comments ...
9 years, 9 months ago (2011-02-17 23:31:03 UTC) #8
merwok
http://codereview.appspot.com/4185044/diff/7003/Doc/library/dbm.rst File Doc/library/dbm.rst (right): http://codereview.appspot.com/4185044/diff/7003/Doc/library/dbm.rst#newcode76 Doc/library/dbm.rst:76: I’d say: The :class:`~collections.MutableMapping` interface is now implemented in ...
9 years, 9 months ago (2011-02-17 23:39:07 UTC) #9
ray
I worked out a new patch and update: 1, Refactoring the common tests between test_dbm_gnu ...
9 years, 9 months ago (2011-02-18 13:09:26 UTC) #10
merwok
9 years, 9 months ago (2011-02-18 13:31:54 UTC) #11
merwok
9 years, 8 months ago (2011-03-20 19:24:29 UTC) #12
merwok
Some more minor comments. Someone with C knowledge will have to review the real code, ...
9 years, 8 months ago (2011-03-20 19:48:08 UTC) #13
merwok
Forgot one thing :) http://codereview.appspot.com/4185044/diff/20001/Lib/test/test_dbm_base.py File Lib/test/test_dbm_base.py (right): http://codereview.appspot.com/4185044/diff/20001/Lib/test/test_dbm_base.py#newcode1 Lib/test/test_dbm_base.py:1: from test import support This ...
9 years, 8 months ago (2011-03-20 19:49:46 UTC) #14
ray
9 years, 8 months ago (2011-03-23 13:24:31 UTC) #15
> Given that your patch is a sizable, non-trivial work, could you send a
contributor agreement?  Details on http://www.python.org/psf/contrib/

Sure, I will.

http://codereview.appspot.com/4185044/diff/20001/Lib/dbm/__init__.py
File Lib/dbm/__init__.py (right):

http://codereview.appspot.com/4185044/diff/20001/Lib/dbm/__init__.py#newcode183
Lib/dbm/__init__.py:183: def _register_with_abc(dbm_open_fun):
On 2011/03/20 19:48:08, merwok wrote:
> Nice way of doing the registration in the absence of exposed types!  I hope
the
> startup cost will be acceptable.
> 
> Want a nitpick?  I use func, not fun, to name a function.

Done.

http://codereview.appspot.com/4185044/diff/20001/Lib/test/test_dbm_base.py
File Lib/test/test_dbm_base.py (right):

http://codereview.appspot.com/4185044/diff/20001/Lib/test/test_dbm_base.py#ne...
Lib/test/test_dbm_base.py:1: from test import support
On 2011/03/20 19:49:46, merwok wrote:
> This file should be not be named test_*, because it does not contain
executable
> tests.  Something like dbm_tests would be good (in mirror of seq_tests).

Done.

http://codereview.appspot.com/4185044/diff/20001/Lib/test/test_dbm_base.py#ne...
Lib/test/test_dbm_base.py:18: self.openner = lambda x=filename:
self._module.open(x, 'c')
On 2011/03/20 19:48:08, merwok wrote:
> Typo: opener
> 
> I find the lambda not very readable, would you mind turning it into a
function?

Done.

http://codereview.appspot.com/4185044/diff/20001/Lib/test/test_dbm_base.py#ne...
Lib/test/test_dbm_base.py:288: self.assertRaises(self._module.error, lambda:
self.d['a'])
On 2011/03/20 19:48:08, merwok wrote:
> I find assertRaises + lambda not very reader-friendly.  Can you rewrite them
to
> use the other form?
> 
> with self.assertRaises(self._module.error):
>     self.d['a']

Done.

http://codereview.appspot.com/4185044/diff/20001/Lib/test/test_dbm_dumb.py
File Lib/test/test_dbm_dumb.py (right):

http://codereview.appspot.com/4185044/diff/20001/Lib/test/test_dbm_dumb.py#ne...
Lib/test/test_dbm_dumb.py:91: f.close()
On 2011/03/20 19:48:08, merwok wrote:
> Shouldn’t this be protected in a try/finally block?

Looks like so. Previously I just left the code as it was. I will put a close()
call into tearDown() method.

http://codereview.appspot.com/4185044/diff/20001/Lib/test/test_dbm_gnu.py
File Lib/test/test_dbm_gnu.py (right):

http://codereview.appspot.com/4185044/diff/20001/Lib/test/test_dbm_gnu.py#new...
Lib/test/test_dbm_gnu.py:31: def test_reorganize(self):
On 2011/03/20 19:48:08, merwok wrote:
> Please don’t move methods, it makes diffs less easy to review.

Done.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b