| msg241875 - (view) |
Author: Behdad Esfahbod (Behdad.Esfahbod) |
Date: 2015-04-23 18:35 |
| Please replace instances of type({}) in plistlib.py with self._dict_type. |
|
|
| msg241895 - (view) |
Author: Ned Deily (ned.deily) *  |
Date: 2015-04-23 23:05 |
| The documentation does not explicitly state whether or not dict_type values have to be instances / subclasses of dict. Can you give a code example, preferably something that could be added to Lib/test/test_plistlib.py, of a use case for something that is not a subclass of dict? Some possible courses of action here: (1) Document that dict_type must be an instance or subclass of dict; (2) Change plistlib as you suggest to allow non-subclasses of dict; (3) Change plistlib to test for a subclass of collections.abc.MutableMapping; (4) other? |
|
|
| msg241897 - (view) |
Author: Behdad Esfahbod (Behdad.Esfahbod) |
Date: 2015-04-23 23:18 |
| I don't have a valid use-case in mind. I was reading the code and noticed this discrepancy. (4) replace the isinstance(self.stack[-1], type({})) with not isinstance(self.stack[-1], type([])). |
|
|
| msg241898 - (view) |
Author: Ned Deily (ned.deily) * |
Date: 2015-04-23 23:40 |
| Sorry, I don't understand your suggested (4). If someone cares to provide a suggested patch (with appropriate tests), we could review it. Otherwise, I would be inclined to close this issue as not an issue. Ronald, any opinion? |
|
|
| msg241899 - (view) |
Author: Behdad Esfahbod (Behdad.Esfahbod) |
Date: 2015-04-23 23:53 |
| The items on the stack are created in two ways: [], and self._dict_type(). Currently the code assumes that self._dict_type() returns an object that passes isinstance(..., type({})). I suggested the following two ways to improve this check: - Replace with: isinstance(..., self._dict_type) - Replace with: not isinstance(..., type([])) Feel free to close. I reported because I saw room for improvement. I don't /need/ the change myself. Thanks. |
|
|
| msg241910 - (view) |
Author: Ronald Oussoren (ronaldoussoren) * |
Date: 2015-04-24 05:13 |
| The test for type({}) is indeed wrong and should check for self._dict_type instead. However, there needs to be a test before that change is made. -- On the road, hence brief. Op 24 apr. 2015 om 01:40 heeft Ned Deily <report@bugs.python.org> het volgende geschreven: > > Ned Deily added the comment: > > Sorry, I don't understand your suggested (4). If someone cares to provide a suggested patch (with appropriate tests), we could review it. Otherwise, I would be inclined to close this issue as not an issue. Ronald, any opinion? > > ---------- > > _______________________________________ > Python tracker <report@bugs.python.org> > <http://bugs.python.org/issue24040> > _______________________________________ |
|
|
| msg242110 - (view) |
Author: Ronald Oussoren (ronaldoussoren) * |
Date: 2015-04-27 11:02 |
| To react to myself: checking for self.dict_type might break users that pass in a callable: x = plistlib.load(fp, dict_type=lambda:{}) As Behdad memtioned testing that the type isn't list would be better: if not isinstance(..., list): That attached patch adds a testcase and removes replaces the test for type({}) for something better. Note: it also replaces ``type([])`` with ``list``, the latter is cleaner. |
|
|
| msg407171 - (view) |
Author: Serhiy Storchaka (serhiy.storchaka) * |
Date: 2021-11-27 21:08 |
| The patch LGTM, and I have nothing to add. Ronald, do you mind to create a PR. |
|
|