Issue 4185044: Improve dbm modules — Python #9523
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 : #
Downloaded from: http://bugs.python.org/file20783/issue_9523.diff
MessagesTotal messages: 15
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 will go into 3.3. http://codereview.appspot.com/4185044/diff/1/Lib/test/test_dbm.py File Lib/test/test_dbm.py (right): http://codereview.appspot.com/4185044/diff/1/Lib/test/test_dbm.py#newcode67 Lib/test/test_dbm.py:67: self.assertTupleEqual((), tuple(f.keys())) You should use assertEqual, it will pick the type-specific comparison automatically. I think you can compare an empty set with f.keys(). http://codereview.appspot.com/4185044/diff/1/Lib/test/test_dbm.py#newcode151 Lib/test/test_dbm.py:151: self.assertTupleEqual((), tuple(self.d.keys())) Ditto. http://codereview.appspot.com/4185044/diff/1/Lib/test/test_dbm_dumb.py File Lib/test/test_dbm_dumb.py (right): http://codereview.appspot.com/4185044/diff/1/Lib/test/test_dbm_dumb.py#newcode95 Lib/test/test_dbm_dumb.py:95: f = dumbdbm.open(_fname) Use a with statement if it’s supported, else a try/finally to make sure close is always called. http://codereview.appspot.com/4185044/diff/1/Lib/test/test_dbm_dumb.py#newcod... Lib/test/test_dbm_dumb.py:184: self.assertEqual(values, dvalues) There is a comparison method that does not take order into account, you could use it to avoid the need to call sorted. http://codereview.appspot.com/4185044/diff/1/Lib/test/test_dbm_gnu.py File Lib/test/test_dbm_gnu.py (right): http://codereview.appspot.com/4185044/diff/1/Lib/test/test_dbm_gnu.py#newcode23 Lib/test/test_dbm_gnu.py:23: self.assertEqual(b'b', self.g.get(b'a')) Is it properly documented that dbm modules will treat strings and bytes as the same thing, or perform a decoding or what it is they’re doing? That smells like 2.x to me. http://codereview.appspot.com/4185044/diff/1/Lib/test/test_dbm_gnu.py#newcode87 Lib/test/test_dbm_gnu.py:87: self.assertEqual(set((b'a', b'c')), set((next(gi), next(gi)))) Why a set and not a tuple here? Is iteration order undefined? (As you can guess, I haven’t checked the doc :) http://codereview.appspot.com/4185044/diff/1/Lib/test/test_dbm_gnu.py#newcode122 Lib/test/test_dbm_gnu.py:122: from collections import Iterable, Mapping, MutableMapping Imports at the function level are a recipe for concurrency bugs. Imports should always be done at the top level. http://codereview.appspot.com/4185044/diff/1/Lib/test/test_dbm_gnu.py#newcode125 Lib/test/test_dbm_gnu.py:125: self.assertIsInstance(self.g, MutableMapping) The first two checks are redundant. http://codereview.appspot.com/4185044/diff/1/Modules/_dbmmodule.c File Modules/_dbmmodule.c (right): http://codereview.appspot.com/4185044/diff/1/Modules/_dbmmodule.c#newcode43 Modules/_dbmmodule.c:43: #define PyDbm_Check(op) PyObject_TypeCheck(op, &Dbmtype) (Note that I know near to nothing about C.) Will that macro leak outside of this file? http://codereview.appspot.com/4185044/diff/1/Modules/_dbmmodule.c#newcode278 Modules/_dbmmodule.c:278: return result; Why this change? http://codereview.appspot.com/4185044/diff/1/Modules/_dbmmodule.c#newcode688 Modules/_dbmmodule.c:688: "keys() -> a new view of dbm keys set.\n" I suggest you reuse the doc for dict.keys and friends (D.keys() -> a set-like object providing a view on D's keys) http://codereview.appspot.com/4185044/diff/1/Modules/_dbmmodule.c#newcode1053 Modules/_dbmmodule.c:1053: mapping_abc = PyObject_GetAttrString(coll_mod, "MutableMapping"); You don’t have to do an explicit registration. By design, a class that implements the documented API will be considered an instance of the ABC. Actually, I think an explicit registration can hurt in this case, because it means that issubclass/isinstance will return true, whereas without explicit registration, they will return true only if the API matches. You may want to get a second advice, I’m only 80% sure.
Sign in to reply to this message.
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 being in rc stage, your additions will go into 3.3. Done. http://codereview.appspot.com/4185044/diff/1/Lib/test/test_dbm.py File Lib/test/test_dbm.py (right): http://codereview.appspot.com/4185044/diff/1/Lib/test/test_dbm.py#newcode67 Lib/test/test_dbm.py:67: self.assertTupleEqual((), tuple(f.keys())) On 2011/02/10 21:53:49, merwok wrote: > You should use assertEqual, it will pick the type-specific comparison > automatically. I think you can compare an empty set with f.keys(). Done. http://codereview.appspot.com/4185044/diff/1/Lib/test/test_dbm.py#newcode151 Lib/test/test_dbm.py:151: self.assertTupleEqual((), tuple(self.d.keys())) On 2011/02/10 21:53:49, merwok wrote: > Ditto. Done. http://codereview.appspot.com/4185044/diff/1/Lib/test/test_dbm_dumb.py File Lib/test/test_dbm_dumb.py (right): http://codereview.appspot.com/4185044/diff/1/Lib/test/test_dbm_dumb.py#newcode95 Lib/test/test_dbm_dumb.py:95: f = dumbdbm.open(_fname) On 2011/02/10 21:53:49, merwok wrote: > Use a with statement if it’s supported, else a try/finally to make sure close is > always called. Currently dbm object are not context managers. And even f.close() is not called due to exception raised after dumbdbm.open(), the resource of database object can still be freed in object's deallocator. So I feel here in unit tests it is not so necessary to make sure close is called. Here are other tests using this style, changing them to using try/finally may go to another issue's patch which is about code style, I guess? But because calling close() immediately is better style, I will add context manager support to dbm objects in another issue later and change these calls to using 'with' statements. http://codereview.appspot.com/4185044/diff/1/Lib/test/test_dbm_dumb.py#newcod... Lib/test/test_dbm_dumb.py:184: self.assertEqual(values, dvalues) On 2011/02/10 21:53:49, merwok wrote: > There is a comparison method that does not take order into account, you could > use it to avoid the need to call sorted. Done. http://codereview.appspot.com/4185044/diff/1/Lib/test/test_dbm_gnu.py File Lib/test/test_dbm_gnu.py (right): http://codereview.appspot.com/4185044/diff/1/Lib/test/test_dbm_gnu.py#newcode23 Lib/test/test_dbm_gnu.py:23: self.assertEqual(b'b', self.g.get(b'a')) On 2011/02/10 21:53:49, merwok wrote: > Is it properly documented that dbm modules will treat strings and bytes as the > same thing, or perform a decoding or what it is they’re doing? That smells like > 2.x to me. I think the example code in dbm document could illustrate this. http://codereview.appspot.com/4185044/diff/1/Lib/test/test_dbm_gnu.py#newcode87 Lib/test/test_dbm_gnu.py:87: self.assertEqual(set((b'a', b'c')), set((next(gi), next(gi)))) On 2011/02/10 21:53:49, merwok wrote: > Why a set and not a tuple here? Is iteration order undefined? (As you can > guess, I haven’t checked the doc :) Yes, the order is not defined. http://codereview.appspot.com/4185044/diff/1/Lib/test/test_dbm_gnu.py#newcode122 Lib/test/test_dbm_gnu.py:122: from collections import Iterable, Mapping, MutableMapping On 2011/02/10 21:53:49, merwok wrote: > Imports at the function level are a recipe for concurrency bugs. Imports should > always be done at the top level. Done. http://codereview.appspot.com/4185044/diff/1/Lib/test/test_dbm_gnu.py#newcode125 Lib/test/test_dbm_gnu.py:125: self.assertIsInstance(self.g, MutableMapping) On 2011/02/10 21:53:49, merwok wrote: > The first two checks are redundant. Done. http://codereview.appspot.com/4185044/diff/1/Modules/_dbmmodule.c File Modules/_dbmmodule.c (right): http://codereview.appspot.com/4185044/diff/1/Modules/_dbmmodule.c#newcode43 Modules/_dbmmodule.c:43: #define PyDbm_Check(op) PyObject_TypeCheck(op, &Dbmtype) On 2011/02/10 21:53:49, merwok wrote: > (Note that I know near to nothing about C.) Will that macro leak outside of > this file? Done. http://codereview.appspot.com/4185044/diff/1/Modules/_dbmmodule.c#newcode278 Modules/_dbmmodule.c:278: return result; On 2011/02/10 21:53:49, merwok wrote: > Why this change? Errr, sorry, my fault. Done. http://codereview.appspot.com/4185044/diff/1/Modules/_dbmmodule.c#newcode688 Modules/_dbmmodule.c:688: "keys() -> a new view of dbm keys set.\n" On 2011/02/10 21:53:49, merwok wrote: > I suggest you reuse the doc for dict.keys and friends (D.keys() -> a set-like > object providing a view on D's keys) Done. http://codereview.appspot.com/4185044/diff/1/Modules/_dbmmodule.c#newcode1053 Modules/_dbmmodule.c:1053: mapping_abc = PyObject_GetAttrString(coll_mod, "MutableMapping"); On 2011/02/10 21:53:49, merwok wrote: > whereas without explicit registration, they will return true only if the API matches. Only those abc who have a __subclasshook__() method will check if API matches, not include Mapping and MappingView. So I need to registration it explicitly.
Sign in to reply to this message.
This version of the patch looks even better! I have no idea about reference counting in CPython, but you can run “./python -m test -R 3:5 test_dbm” to find refleaks. I assume your python also uses pydebug, to find resource (file) leaks. 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. Please add a versionchanged entry for 3.3 without removing the one existing in 3.2 :) http://codereview.appspot.com/4185044/diff/7002/Lib/dbm/dumb.py File Lib/dbm/dumb.py (right): http://codereview.appspot.com/4185044/diff/7002/Lib/dbm/dumb.py#newcode210 Lib/dbm/dumb.py:210: BTW, a speed-up trick used by Raymond in OrderedDict is to locally alias “keys = collections.MutableMapping.keys”, to save up method lookup at runtime. (Just FYI, I can’t say whether it’s a useful trick or premature optimization.) http://codereview.appspot.com/4185044/diff/7002/Lib/test/test_dbm.py File Lib/test/test_dbm.py (right): http://codereview.appspot.com/4185044/diff/7002/Lib/test/test_dbm.py#newcode67 Lib/test/test_dbm.py:67: self.assertEqual(f.keys(), set()) Yay! http://codereview.appspot.com/4185044/diff/7002/Lib/test/test_dbm_gnu.py File Lib/test/test_dbm_gnu.py (right): http://codereview.appspot.com/4185044/diff/7002/Lib/test/test_dbm_gnu.py#newc... Lib/test/test_dbm_gnu.py:88: self.assertRaises(StopIteration, next, iter(self.g)) FYI, there are two styles for assertRaises: either the one you’re using here, or a context manager form which makes it easier to write regular-looking code: with self.assertRaises(StopIteration): next(iter(self.g)) Use whichever makes things easier for you. http://codereview.appspot.com/4185044/diff/7002/Lib/test/test_dbm_gnu.py#newc... Lib/test/test_dbm_gnu.py:119: self.assertFalse(keys.isdisjoint({b'a', b'e', b'f'})) Dict views also implement the magic methods allowing to do set operations (^, |, &). http://codereview.appspot.com/4185044/diff/7002/Lib/test/test_dbm_gnu.py#newc... Lib/test/test_dbm_gnu.py:153: def test_abc(self): That’s a good way to test for API conformance (in method existence at least, not behavior, but let’s take what we can get for free), provided that you do not explicitly register the classes. Two remarks: The method could have a clearer name, maybe test_views_conformance; if this code is repeated for every dbm test module, you could make a TestDbmBase class to factor out the common methods in only one place. (Sorry for not thinking to mention that earlier!) http://codereview.appspot.com/4185044/diff/7002/Lib/test/test_dbm_gnu.py#newc... Lib/test/test_dbm_gnu.py:193: class Other(object): No need to explicitly inherit from object in 3.x. We’ve removed it from the docs, let’s do the same in code :) http://codereview.appspot.com/4185044/diff/7002/Lib/test/test_dbm_ndbm.py File Lib/test/test_dbm_ndbm.py (right): http://codereview.appspot.com/4185044/diff/7002/Lib/test/test_dbm_ndbm.py#new... Lib/test/test_dbm_ndbm.py:44: self.assertEqual(set(self.d.values()), set()) Do you have to convert d.values() to a set? Isn’t it already a dict view, which is conceptually a set? http://codereview.appspot.com/4185044/diff/7002/Lib/test/test_dbm_ndbm.py#new... Lib/test/test_dbm_ndbm.py:58: self.assertEqual(2, len(self.d.items())) There is no enforced convention about the order of arguments in assert* methods: some people use assertEqual(expected_value, compute_value()), other people use the reverse. It should be consistent in one module, though, to ease debugging (when reading the error diff, you need to know where the computed and expected values are). IOW, assertEqual(len(self.d.items()), 2) http://codereview.appspot.com/4185044/diff/7002/Modules/_dbmmodule.c File Modules/_dbmmodule.c (right): http://codereview.appspot.com/4185044/diff/7002/Modules/_dbmmodule.c#newcode42 Modules/_dbmmodule.c:42: PyTypeObject PyDbmKeys_Type; Remembering that I don’t know C nor the Python rules for public names, is it okay that this starts with Py? Also, how does it play with the PEP 384 limited API? http://codereview.appspot.com/4185044/diff/7002/Modules/_dbmmodule.c#newcode611 Modules/_dbmmodule.c:611: "Return a set-like object providing a view on database's keys."}, “database’s keys” seems wrong. The dict method doc includes a name for the instance: “D.keys() -> a set-like object providing a view on D's keys” http://codereview.appspot.com/4185044/diff/7002/Modules/_gdbmmodule.c File Modules/_gdbmmodule.c (right): http://codereview.appspot.com/4185044/diff/7002/Modules/_gdbmmodule.c#newcode431 Modules/_gdbmmodule.c:431: PyErr_SetString(PyExc_KeyError, "GDBM object has no more items"); Why caps all of a sudden?
Sign in to reply to this message.
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 add a versionchanged entry for 3.3 without removing the one existing in > 3.2 :) Done. http://codereview.appspot.com/4185044/diff/7002/Lib/dbm/dumb.py File Lib/dbm/dumb.py (right): http://codereview.appspot.com/4185044/diff/7002/Lib/dbm/dumb.py#newcode210 Lib/dbm/dumb.py:210: On 2011/02/12 20:14:50, merwok wrote: > BTW, a speed-up trick used by Raymond in OrderedDict is to locally alias “keys = > collections.MutableMapping.keys”, to save up method lookup at runtime. (Just > FYI, I can’t say whether it’s a useful trick or premature optimization.) Did you mean: """ def items(self): return [(key, self[key]) for key in self._index.keys()] """ ? Yes, I believe that is a useful trick. http://codereview.appspot.com/4185044/diff/7002/Lib/test/test_dbm_gnu.py File Lib/test/test_dbm_gnu.py (right): http://codereview.appspot.com/4185044/diff/7002/Lib/test/test_dbm_gnu.py#newc... Lib/test/test_dbm_gnu.py:88: self.assertRaises(StopIteration, next, iter(self.g)) On 2011/02/12 20:14:50, merwok wrote: > FYI, there are two styles for assertRaises: either the one you’re using here, or > a context manager form which makes it easier to write regular-looking code: > > with self.assertRaises(StopIteration): > next(iter(self.g)) > > Use whichever makes things easier for you. Good! That style looks very nice! Thx! I'll use it in later python codes.~~ But to keep consist with existing tests, I will use my one. http://codereview.appspot.com/4185044/diff/7002/Lib/test/test_dbm_gnu.py#newc... Lib/test/test_dbm_gnu.py:119: self.assertFalse(keys.isdisjoint({b'a', b'e', b'f'})) On 2011/02/12 20:14:50, merwok wrote: > Dict views also implement the magic methods allowing to do set operations (^, |, > &). Done. I will implement the same methods in dbm. http://codereview.appspot.com/4185044/diff/7002/Lib/test/test_dbm_gnu.py#newc... Lib/test/test_dbm_gnu.py:153: def test_abc(self): On 2011/02/12 20:14:50, merwok wrote: > That’s a good way to test for API conformance (in method existence at least, not > behavior, but let’s take what we can get for free), provided that you do not > explicitly register the classes. > > Two remarks: The method could have a clearer name, maybe test_views_conformance; > if this code is repeated for every dbm test module, you could make a TestDbmBase > class to factor out the common methods in only one place. (Sorry for not > thinking to mention that earlier!) Here test_abc() I means test weather dbm objects are subclasses of abc, not their api comformance. (Exactly, I just test the abc registering.) Mapping abc and MappingView abc don't have __subclasshook__() method, so I don't know how to test API conformance(at least method exists). These are currently tested by each of methods in other tests. There could be much tests repeated for every dbm test module, including existing tests and the newly added. But again I prefer put this refactoring into another separate issue, since I don't want do too much things in one issue's patch. Doing this could cause another a lot of changes. What about you opinion? http://codereview.appspot.com/4185044/diff/7002/Lib/test/test_dbm_gnu.py#newc... Lib/test/test_dbm_gnu.py:193: class Other(object): On 2011/02/12 20:14:50, merwok wrote: > No need to explicitly inherit from object in 3.x. We’ve removed it from the > docs, let’s do the same in code :) Done. http://codereview.appspot.com/4185044/diff/7002/Lib/test/test_dbm_ndbm.py File Lib/test/test_dbm_ndbm.py (right): http://codereview.appspot.com/4185044/diff/7002/Lib/test/test_dbm_ndbm.py#new... Lib/test/test_dbm_ndbm.py:44: self.assertEqual(set(self.d.values()), set()) On 2011/02/12 20:14:50, merwok wrote: > Do you have to convert d.values() to a set? Isn’t it already a dict view, which > is conceptually a set? In fact dict values view is not conceptually set as keys view and items view, it could just be a sized, iterable container, since dict values may have duplicate items. It has not compare methods and other set operations. http://codereview.appspot.com/4185044/diff/7002/Lib/test/test_dbm_ndbm.py#new... Lib/test/test_dbm_ndbm.py:58: self.assertEqual(2, len(self.d.items())) On 2011/02/12 20:14:50, merwok wrote: > There is no enforced convention about the order of arguments in assert* methods: > some people use assertEqual(expected_value, compute_value()), other people use > the reverse. It should be consistent in one module, though, to ease debugging > (when reading the error diff, you need to know where the computed and expected > values are). IOW, assertEqual(len(self.d.items()), 2) Done. http://codereview.appspot.com/4185044/diff/7002/Modules/_dbmmodule.c File Modules/_dbmmodule.c (right): http://codereview.appspot.com/4185044/diff/7002/Modules/_dbmmodule.c#newcode42 Modules/_dbmmodule.c:42: PyTypeObject PyDbmKeys_Type; On 2011/02/12 20:14:50, merwok wrote: > Remembering that I don’t know C nor the Python rules for public names, is it > okay that this starts with Py? Also, how does it play with the PEP 384 limited > API? No, it isn't. The pep 7 said "Py" prefix should only be used with public names. I will fix myself. http://codereview.appspot.com/4185044/diff/7002/Modules/_dbmmodule.c#newcode611 Modules/_dbmmodule.c:611: "Return a set-like object providing a view on database's keys."}, On 2011/02/12 20:14:50, merwok wrote: > “database’s keys” seems wrong. The dict method doc includes a name for the > instance: “D.keys() -> a set-like object providing a view on D's keys” I didn't use an instance name here is to keep consist with existing method doc style. Just like the doc of get() method below. http://codereview.appspot.com/4185044/diff/7002/Modules/_gdbmmodule.c File Modules/_gdbmmodule.c (right): http://codereview.appspot.com/4185044/diff/7002/Modules/_gdbmmodule.c#newcode431 Modules/_gdbmmodule.c:431: PyErr_SetString(PyExc_KeyError, "GDBM object has no more items"); On 2011/02/12 20:14:50, merwok wrote: > Why caps all of a sudden? Sorry, I didn't understand this sentence. I know nothing about this word: "caps". Could you explain it more clearly? Thanks~~
Sign in to reply to this message.
I replied without quoting text, you can go to the codereview page to read my comments in context. http://codereview.appspot.com/4185044/diff/7002/Lib/dbm/dumb.py File Lib/dbm/dumb.py (right): http://codereview.appspot.com/4185044/diff/7002/Lib/dbm/dumb.py#newcode210 Lib/dbm/dumb.py:210: No, I meant exactly what I wrote: class WhateverMapping(MutableMapping): keys = MutableMapping.keys values = MutableMapping.values The speed-up comes from the fact that the methods will be found on the WhateverMapping class, Python won’t have to go to the parent class to find it. Use the Source, Luke: http://code.python.org/hg/branches/py3k/file/4df586851b0f/Lib/collections.py#... Again, I don’t know whether it’s a good idea or a premature optimization. Raymond knows. http://codereview.appspot.com/4185044/diff/7002/Lib/test/test_dbm_gnu.py File Lib/test/test_dbm_gnu.py (right): http://codereview.appspot.com/4185044/diff/7002/Lib/test/test_dbm_gnu.py#newc... Lib/test/test_dbm_gnu.py:119: self.assertFalse(keys.isdisjoint({b'a', b'e', b'f'})) Great. I’m sure you won’t forget to test those methods. http://codereview.appspot.com/4185044/diff/7002/Lib/test/test_dbm_gnu.py#newc... Lib/test/test_dbm_gnu.py:135: def test_values(self): Useful invariants to test, from PEP 3119: (m is a mapping instance, so here a dbm instance) len(m.values()) == len(m.keys()) == len(m.items()) == len(m) [value for value in m.values()] == [m[key] for key in m.keys()] [item for item in m.items()] == [(key, m[key]) for key in m.keys()] http://codereview.appspot.com/4185044/diff/7002/Lib/test/test_dbm_gnu.py#newc... Lib/test/test_dbm_gnu.py:153: def test_abc(self): I replied about the ABCs on the Roundup issue (summary: you’re right). About the tests refactor: It’s not uncommon to find that tests need refactoring while adding features or fixing a serious bug. If it makes reviewing hard, you can first make a patch that refactors the tests without touching the code at all, and then another patch to add features and tests. Factoring out common code is best for all parties IMO: It sparse you the writing of identical code (with risks of typos, etc.) and saves some review time. http://codereview.appspot.com/4185044/diff/7002/Lib/test/test_dbm_ndbm.py File Lib/test/test_dbm_ndbm.py (right): http://codereview.appspot.com/4185044/diff/7002/Lib/test/test_dbm_ndbm.py#new... Lib/test/test_dbm_ndbm.py:44: self.assertEqual(set(self.d.values()), set()) You’re once again right :) I had read too fast; I checked again in PEP 3106, Lib/test/test_dictviews.py and Lib/_abcoll.py and found that keys and items views are sets*, but not values view. (* not instances of the built-in set class, but registered with the collections.Set ABC, so that they can be compared with sets) http://codereview.appspot.com/4185044/diff/7002/Modules/_dbmmodule.c File Modules/_dbmmodule.c (right): http://codereview.appspot.com/4185044/diff/7002/Modules/_dbmmodule.c#newcode611 Modules/_dbmmodule.c:611: "Return a set-like object providing a view on database's keys."}, All right. Keep the style consistent then, use “keys() -> etc.”, but find a way to make the rest of the text correct, for example: “Return a set-like object providing a view on the database's keys.” (notice I added “the” to refer to the instance). http://codereview.appspot.com/4185044/diff/7002/Modules/_dbmmodule.c#newcode1279 Modules/_dbmmodule.c:1279: PyObject *coll_mod, *mapping_abc, *keysview_abc, *valuesview_abc, I’ve just realized that you can avoid this whole boilerplate by registering the classes in dbm/dbm.py (same for _gdbm/gdbm.py). http://codereview.appspot.com/4185044/diff/7002/Modules/_gdbmmodule.c File Modules/_gdbmmodule.c (right): http://codereview.appspot.com/4185044/diff/7002/Modules/_gdbmmodule.c#newcode431 Modules/_gdbmmodule.c:431: PyErr_SetString(PyExc_KeyError, "GDBM object has no more items"); Caps means capitalized letters, here “GDBM” instead of “gdbm”. I was asking why this error message was using upper case instead of lower case, which I thought was the usual form. I noticed since then that case is inconsistent in the docstrings and error messages, sometimes in the same function! You can use whatever you prefer. http://codereview.appspot.com/4185044/diff/7002/Modules/_gdbmmodule.c#newcode438 Modules/_gdbmmodule.c:438: assert(v.dptr != NULL); I don’t know how asserts work in C. I know that I avoid them in Python to make sure my code style does safety checks when run under python -O. Does it work in a similar way in C or is it okay to use asserts because they’re always executed? http://codereview.appspot.com/4185044/diff/7002/Modules/_gdbmmodule.c#newcode687 Modules/_gdbmmodule.c:687: "else, as well as those in keywords."); The phrasing is a bit awkward.
Sign in to reply to this message.
Thanks again for such comments in details. And please forgive my sometimes English misunderstanding and not-understand. :) http://codereview.appspot.com/4185044/diff/7002/Lib/dbm/dumb.py File Lib/dbm/dumb.py (right): http://codereview.appspot.com/4185044/diff/7002/Lib/dbm/dumb.py#newcode210 Lib/dbm/dumb.py:210: On 2011/02/16 16:34:05, merwok wrote: > No, I meant exactly what I wrote: > > class WhateverMapping(MutableMapping): > > keys = MutableMapping.keys > values = MutableMapping.values > > The speed-up comes from the fact that the methods will be found on the > WhateverMapping class, Python won’t have to go to the parent class to find it. > Use the Source, Luke: > http://code.python.org/hg/branches/py3k/file/4df586851b0f/Lib/collections.py#... > > Again, I don’t know whether it’s a good idea or a premature optimization. > Raymond knows. Oh I understand. I do feel for dumb this is not so needed since dumb is not intended to written for speed. Here is the doc in dbm module library: "The dbm.dumb module is not written for speed and is not nearly as heavily used as the other database modules." Just my own opinion~ :) http://codereview.appspot.com/4185044/diff/7002/Lib/test/test_dbm_gnu.py File Lib/test/test_dbm_gnu.py (right): http://codereview.appspot.com/4185044/diff/7002/Lib/test/test_dbm_gnu.py#newc... Lib/test/test_dbm_gnu.py:119: self.assertFalse(keys.isdisjoint({b'a', b'e', b'f'})) On 2011/02/16 16:34:05, merwok wrote: > Great. I’m sure you won’t forget to test those methods. No, I have implemented them in my latest patch with tests already. http://codereview.appspot.com/4185044/diff/7002/Lib/test/test_dbm_gnu.py#newc... Lib/test/test_dbm_gnu.py:135: def test_values(self): On 2011/02/16 16:34:05, merwok wrote: > Useful invariants to test, from PEP 3119: > > (m is a mapping instance, so here a dbm instance) > > len(m.values()) == len(m.keys()) == len(m.items()) == len(m) > [value for value in m.values()] == [m[key] for key in m.keys()] > [item for item in m.items()] == [(key, m[key]) for key in m.keys()] Looks good, though I have tested each operation in detail, but add these tests as overall tests seems reasonable. I will add them. http://codereview.appspot.com/4185044/diff/7002/Lib/test/test_dbm_gnu.py#newc... Lib/test/test_dbm_gnu.py:153: def test_abc(self): On 2011/02/16 16:34:05, merwok wrote: > If it makes reviewing hard, you can first make a patch that refactors the tests without touching the code at all, and then another patch to add features and tests. By scanning the code again, I found all the three modules test_dbm_gnu, test_dbm_ndbm, test_dbm_dumb could have mostly same tests only after applying my patch. So I will refactor the tests in my patch. http://codereview.appspot.com/4185044/diff/7002/Lib/test/test_dbm_ndbm.py File Lib/test/test_dbm_ndbm.py (right): http://codereview.appspot.com/4185044/diff/7002/Lib/test/test_dbm_ndbm.py#new... Lib/test/test_dbm_ndbm.py:44: self.assertEqual(set(self.d.values()), set()) On 2011/02/16 16:34:05, merwok wrote: > You’re once again right :) I had read too fast; I checked again in PEP 3106, > Lib/test/test_dictviews.py and Lib/_abcoll.py and found that keys and items > views are sets*, but not values view. > > (* not instances of the built-in set class, but registered with the > collections.Set ABC, so that they can be compared with sets) Yes. And dict value views doesn't have richcompare functions. http://codereview.appspot.com/4185044/diff/7002/Modules/_dbmmodule.c File Modules/_dbmmodule.c (right): http://codereview.appspot.com/4185044/diff/7002/Modules/_dbmmodule.c#newcode42 Modules/_dbmmodule.c:42: PyTypeObject PyDbmKeys_Type; On 2011/02/16 14:44:14, ray wrote: > On 2011/02/12 20:14:50, merwok wrote: > > Remembering that I don’t know C nor the Python rules for public names, is it > > okay that this starts with Py? Also, how does it play with the PEP 384 > limited > > API? > Then there are no public api in this module. If I understand correctly? http://codereview.appspot.com/4185044/diff/7002/Modules/_dbmmodule.c#newcode611 Modules/_dbmmodule.c:611: "Return a set-like object providing a view on database's keys."}, On 2011/02/16 16:34:05, merwok wrote: > All right. Keep the style consistent then, use “keys() -> etc.”, but find a way > to make the rest of the text correct, for example: “Return a set-like object > providing a view on the database's keys.” (notice I added “the” to refer to the > instance). Oh, yes, I misunderstand your previous comment's meaning~ :) done. http://codereview.appspot.com/4185044/diff/7002/Modules/_dbmmodule.c#newcode1279 Modules/_dbmmodule.c:1279: PyObject *coll_mod, *mapping_abc, *keysview_abc, *valuesview_abc, On 2011/02/16 16:34:05, merwok wrote: > I’ve just realized that you can avoid this whole boilerplate by registering the > classes in dbm/dbm.py (same for _gdbm/gdbm.py). gdbm and ndbm modules are not builtin modules and not needed in every python program which need abc module. Doing this could cause every program which import abc module also import dbm and gdbm module. So is this a little waste? Hmm, just my own feeling~~ http://codereview.appspot.com/4185044/diff/7002/Modules/_gdbmmodule.c File Modules/_gdbmmodule.c (right): http://codereview.appspot.com/4185044/diff/7002/Modules/_gdbmmodule.c#newcode438 Modules/_gdbmmodule.c:438: assert(v.dptr != NULL); On 2011/02/16 16:34:05, merwok wrote: > I don’t know how asserts work in C. I know that I avoid them in Python to make > sure my code style does safety checks when run under python -O. Does it work in > a similar way in C or is it okay to use asserts because they’re always executed? It work in a similar way in C. By defining the macro NDEBUG all the assert won't work. http://codereview.appspot.com/4185044/diff/7002/Modules/_gdbmmodule.c#newcode687 Modules/_gdbmmodule.c:687: "else, as well as those in keywords."); On 2011/02/16 16:34:05, merwok wrote: > The phrasing is a bit awkward. How about this: """ Add all the items in keywords to the databases. If other is a mapping object, does: for k in other: self[k] = other[k] Else if other has a "keys()" method, does: for k in other.keys(): self[k] = other[k] Else does: for k, v in other: self[k] = v """ Just follow dict's update() method doc way.
Sign in to reply to this message.
> Thanks again for such comments in details. Thank you for responding to my comments and nitpicks! And the C code has not even been reviewed yet <wink> > And please forgive my sometimes English misunderstanding > and not-understand. :) There is nothing to forgive; language mastery is not a requirement at all. See http://ostatic.com/blog/more-bad-english-please http://codereview.appspot.com/4185044/diff/7002/Lib/dbm/dumb.py File Lib/dbm/dumb.py (right): http://codereview.appspot.com/4185044/diff/7002/Lib/dbm/dumb.py#newcode210 Lib/dbm/dumb.py:210: Right, let’s prefer clean code over fast code here. http://codereview.appspot.com/4185044/diff/7002/Modules/_dbmmodule.c File Modules/_dbmmodule.c (right): http://codereview.appspot.com/4185044/diff/7002/Modules/_dbmmodule.c#newcode42 Modules/_dbmmodule.c:42: PyTypeObject PyDbmKeys_Type; The public functions that were here before your patch are still here. The functions that you added as implementation details don’t change that. http://codereview.appspot.com/4185044/diff/7002/Modules/_dbmmodule.c#newcode1279 Modules/_dbmmodule.c:1279: PyObject *coll_mod, *mapping_abc, *keysview_abc, *valuesview_abc, You got me wrong. What I meant is to do something akin to what Lib/_abcoll.py does: Instead of registering with the ABCs in the private C modules, you can register in the public Python modules, like this: Index: Lib/dbm/gnu.py =================================================================== --- Lib/dbm/gnu.py (révision 88415) +++ Lib/dbm/gnu.py (copie de travail) @@ -1,3 +1,9 @@ """Provide the _gdbm module as a dbm submodule.""" from _gdbm import * +from collections import MutableMapping # etc. + +_db = open(...) +MutableMapping.register(type(_db)) + +del _db, MutableMapping The fact that the C modules only exports one function makes it a bit awkward to register the db object and its dict views, though. Do what’s easier and more readable! http://codereview.appspot.com/4185044/diff/7002/Modules/_gdbmmodule.c File Modules/_gdbmmodule.c (right): http://codereview.appspot.com/4185044/diff/7002/Modules/_gdbmmodule.c#newcode438 Modules/_gdbmmodule.c:438: assert(v.dptr != NULL); Okay. In Python code, I always avoid asserts, because I don’t want a check to be skipped under -O. I don’t know what’s the rule for the C files in CPython, but I’d prefer to use a check that always runs instead of an assert. http://codereview.appspot.com/4185044/diff/7002/Modules/_gdbmmodule.c#newcode687 Modules/_gdbmmodule.c:687: "else, as well as those in keywords."); Yes, following the doc for dict.update is easier. You can also use code instead of good English (like dict.update). I notice that the behavior of dbm.update is a bit different than that of dict.update; is that on purpose?
Sign in to reply to this message.
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 all database modules. http://codereview.appspot.com/4185044/diff/7003/Lib/test/test_dbm.py File Lib/test/test_dbm.py (right): http://codereview.appspot.com/4185044/diff/7003/Lib/test/test_dbm.py#newcode126 Lib/test/test_dbm.py:126: delete_files() Where does that function come from? http://codereview.appspot.com/4185044/diff/7003/Lib/test/test_dbm_dumb.py File Lib/test/test_dbm_dumb.py (right): http://codereview.appspot.com/4185044/diff/7003/Lib/test/test_dbm_dumb.py#new... Lib/test/test_dbm_dumb.py:191: self.assertEqual(len(items), len(ditems)) That len check is redundant.
Sign in to reply to this message.
I worked out a new patch and update: 1, Refactoring the common tests between test_dbm_gnu and test_dbm_ndbm. I didn't combine test_dbm_dumb because there are several implementation details different in dumb and the two c-implemented modules so some tests needs to be different, and also dumb need not to test many of the collections.Mapping interfaces. 2, Move the abc registering in Lib/dbm/ndbm.py and Lib/dbm/gnu.py. 3, Other changes pointed out in review comments. Since I can't find where to upload a new patch set I just upload my updated patch on http://bugs.python.com/issue9523. http://codereview.appspot.com/4185044/diff/7002/Modules/_dbmmodule.c File Modules/_dbmmodule.c (right): http://codereview.appspot.com/4185044/diff/7002/Modules/_dbmmodule.c#newcode42 Modules/_dbmmodule.c:42: PyTypeObject PyDbmKeys_Type; On 2011/02/17 23:31:03, merwok wrote: > The public functions that were here before your patch are still here. The > functions that you added as implementation details don’t change that. I fixed them in my latest patch. http://codereview.appspot.com/4185044/diff/7002/Modules/_dbmmodule.c#newcode1279 Modules/_dbmmodule.c:1279: PyObject *coll_mod, *mapping_abc, *keysview_abc, *valuesview_abc, On 2011/02/17 23:31:03, merwok wrote: > You got me wrong. What I meant is to do something akin to what Lib/_abcoll.py > does: Instead of registering with the ABCs in the private C modules, you can > register in the public Python modules, like this: > > Index: Lib/dbm/gnu.py > =================================================================== > --- Lib/dbm/gnu.py (révision 88415) > +++ Lib/dbm/gnu.py (copie de travail) > @@ -1,3 +1,9 @@ > """Provide the _gdbm module as a dbm submodule.""" > > from _gdbm import * > +from collections import MutableMapping # etc. > + > +_db = open(...) > +MutableMapping.register(type(_db)) > + > +del _db, MutableMapping > > > The fact that the C modules only exports one function makes it a bit awkward to > register the db object and its dict views, though. Do what’s easier and more > readable! Oh, yes! How can I forget this file and misunderstand your previous comment! Done. http://codereview.appspot.com/4185044/diff/7002/Modules/_gdbmmodule.c File Modules/_gdbmmodule.c (right): http://codereview.appspot.com/4185044/diff/7002/Modules/_gdbmmodule.c#newcode438 Modules/_gdbmmodule.c:438: assert(v.dptr != NULL); On 2011/02/17 23:31:03, merwok wrote: > Okay. In Python code, I always avoid asserts, because I don’t want a check to > be skipped under -O. I don’t know what’s the rule for the C files in CPython, > but I’d prefer to use a check that always runs instead of an assert. In a normal python build, the NDEBUG macro will be defined, and all the asserts don't work. The asserts work only in a debug build. http://codereview.appspot.com/4185044/diff/7002/Modules/_gdbmmodule.c#newcode687 Modules/_gdbmmodule.c:687: "else, as well as those in keywords."); On 2011/02/17 23:31:03, merwok wrote: > Yes, following the doc for dict.update is easier. You can also use code instead > of good English (like dict.update). > > I notice that the behavior of dbm.update is a bit different than that of > dict.update; is that on purpose? Yes, update() is implemented follow the MutableMapping's update() method but not the dict's update(). I asked about the update() interface definition on http://bugs.python.com/issue9523 since the PEP3119 lacks of it, and r.david.murray said the ABC is considered authoritative. I think it sounds reasonable. 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: On 2011/02/17 23:39:07, merwok wrote: > I’d say: > > The :class:`~collections.MutableMapping` interface is now > implemented in all database modules. Done. http://codereview.appspot.com/4185044/diff/7003/Lib/test/test_dbm.py File Lib/test/test_dbm.py (right): http://codereview.appspot.com/4185044/diff/7003/Lib/test/test_dbm.py#newcode126 Lib/test/test_dbm.py:126: delete_files() On 2011/02/17 23:39:07, merwok wrote: > Where does that function come from? It is defined in this module. It deletes all the files that may be produced by ndbm, gdbm and dumb. This change makes test_dbm.py be able to run pass with -R, or it will fail at test_whichdb() since at the second run previous dbm files are not cleaned clearly so the gnu.open() will open an existing dumb database file and the whichdb will tell it is a dumb dbm. http://codereview.appspot.com/4185044/diff/7003/Lib/test/test_dbm_dumb.py File Lib/test/test_dbm_dumb.py (right): http://codereview.appspot.com/4185044/diff/7003/Lib/test/test_dbm_dumb.py#new... Lib/test/test_dbm_dumb.py:191: self.assertEqual(len(items), len(ditems)) On 2011/02/17 23:39:07, merwok wrote: > That len check is redundant. Done.
Sign in to reply to this message.
Some more minor comments. Someone with C knowledge will have to review the real code, maybe you could ask on python-dev. Given that your patch is a sizable, non-trivial work, could you send a contributor agreement? Details on http://www.python.org/psf/contrib/ 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): 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. 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:18: self.openner = lambda x=filename: self._module.open(x, 'c') Typo: opener I find the lambda not very readable, would you mind turning it into a function? 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']) 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'] 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() Shouldn’t this be protected in a try/finally block? 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): Please don’t move methods, it makes diffs less easy to review.
Sign in to reply to this message.
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#ne... Lib/test/test_dbm_base.py:1: from test import support 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).
Sign in to reply to this message.
> 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.
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
