-
-
Notifications
You must be signed in to change notification settings - Fork 30.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Improve dbm modules #53732
Comments
During the patching work of bpo-8634, I found several problems about the module dbm.gnu and dbm.ndbm, I feel it's better to address them on a separate issue.
The patch contains also unittest and doc. |
Upgrading to match the MutableMapping interface seems reasonable. |
In 3.2, objects return by dbm.dumb.open implement MutableMapping with incorrect semantics: keys return a list, iterkeys exist, etc. |
Oh, yes. I noticed that the PEP-3119 defines return value of method MutableMapping.keys() as Set, as well as method items(). So the implementation of dumb.keys() and dump.items() are not correct since they all return lists while the class inherits MutableMapping. The implementations in my patch should also be corrected since I made the same mistake. Besides, since bpo-6045 has already added get(), I need to update my patch. I will do it later. And who can tell the specification of MutableMapping.update()? The PEP-3119 lacks of it. Should I follow the implementation in the ABC class MutableMapping? |
I believe that in the absence of other documentation the ABC is considered authoritative. |
Here is the updated patch, which fixed:
|
Thank you for working on a patch, especially with such comprehensive tests.
The previous text was okay, I wouldn’t have changed it.
I don’t know if you should use a plain set or a collections.ItemsView here. In dict objects, KeysView and ValuesView are set-like objects with added behavior, for example they yield their elements in the same order. Raymond, can you comment? Style remarks: you can iter without calling _index.keys(); you can avoid the intermediary list (IOW, remove enclosing [ and ]). In the tests, you can use specialized methods like assertIn and assertIsNone, they remove some boilerplate and can give better error output. I can’t review the C code. :) |
See bpo-5736 for a patch adding iteration support. If the patch attached to his report supersedes the other one, I’ll close the other bug as duplicate. |
Thanks eric for reviewing my patch! And thanks for you suggestions. I'm following them.
Yes you are right. I think returning a view object is better than returning a set. Here is the updated patch. It updates:
bpo-5736 's patch for adding iteration to ndbm and gdbm modules simple calling PyObject_GetIter(dbm_keys(dbm, NULL)) for both gdbm and ndbm, but I feel it's better to create a seperate iterator object for gdbm objects. |
I can’t judge the C code; maybe Raymond or Daniel will. They’re also experts about collections and ABCs, so they’ll be able to confirm or infirm what I’ve said about dict views and the registration stuff (on codereview). |
I feel not so easy to reuse the code, there could be several differences in the c code. Resuing the code may make the code more complecated. But if someone could find a better way to reuse the code, it would be nice.
Oh, yes, I missed the rich compare functions and isdisjoint() method of view objects.
Here is the updated patch:
|
Other comments on Rietveld (the code review site). BTW, if you’re posting updated patches on Rietveld you can remove patches from this bug report, so that people don’t review old patches. It would also be simpler to keep all discussion there :) |
Closed bpo-5736 as superseded. Please make sure the comments there about the peculiar API of gdbm don’t come up or are addressed in your patch. |
Thanks! Here is my updated patch: |
Thank you for repeating that many times and politely: I was indeed wrong. I went back to PEP-3119 to read again about __instancecheck__ and __subclasscheck__, then experimented in a shell and was surprised. I have been misunderstanding one thing: issubclass(cls, abc) does not return true automatically if cls provides the methods, it’s entirely up to the ABC to check methods or do something else in its __subclasscheck__ or __instancecheck__ methods. (I should have known better, I was in a discussion about adding that very feature on python-ideas and bpo-9731!) After another bit of experimentation with dict views and collections ABCs, I finally understand that you have to register your view classes. Thanks for letting me correct my misunderstanding. |
An updated patch, based on latest several reviews on http://codereview.appspot.com/4185044/ update: |
Sine r88451 removed unittest's assertSameElements() method, I need to updated my patch to fit it. So here it is. |
I think the patch will not be suitable for 3.1 and 3.2, so there should be a doc patch to mention the limitations of the dbm API (keys() returning a list and all that). |
Yes, it changes some api(e.g keys()), which may introduces compatibility issues.
Do you mean a doc patch for 3.2 which mentions the missing or imperfect methods of collections.MutableSequence()? e.g keys() not returns a KeysView but a list instead, update() is missing |
Yes, I mean exactly that. |
Updated patch: 1, Changes follows review comments: http://codereview.appspot.com/4185044/. Thanks eric! 2, Make Objects/dictobject.c:all_contained_in() a common useful limited api Object/abstract.c:_PyObject_AllContainedIn() for the purpose of re-usage in Modules/_gdbmmodule.c and Modules/_dbmmodule.c. Not sure if this is proper. I will ask somebody with C knowledge to do a review on the C code. |
I tried to work out a doc patch for 3.2 to mention the limitation api: the missing methods compared with dict and the imperfect methods(keys(), items()) of collections.MutableMapping. Here is it. |
Updated patch following C code review comments from http://bugs.python.org/review/9523/show Two big changes:
|
Sorry, previous patch(issue_9523_4.diff) missed a file(Lib/test/dbm_tests.py) |
Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.
Show more details
GitHub fields:
bugs.python.org fields:
The text was updated successfully, but these errors were encountered: