| msg336053 - (view) |
Author: Zahash Z (Zahash Z) * |
Date: 2019-02-20 10:14 |
| There is no __repr__() method for the PriorityQueue class and LifoQueue class in the queue.py file This makes it difficult to check the elements of the queue. |
|
|
| msg336054 - (view) |
Author: Stéphane Wirtel (matrixise) *  |
Date: 2019-02-20 10:21 |
| but the __repr__ would have a limitation because you can't show all the elements from your queue, for example, if your queue contains 1000 items, the __repr__ will show the total items or just the ten first ones? We need a compromise for that. |
|
|
| msg336055 - (view) |
Author: Stéphane Wirtel (matrixise) * |
Date: 2019-02-20 10:23 |
| and there is an other issue, we need to iterate over the elements of the deque() :/ not really performant, just for a repr(). for my part, -1 if we have to iterate over the elements. |
|
|
| msg336057 - (view) |
Author: Windson Yang (Windson Yang) * |
Date: 2019-02-20 10:31 |
| Hello, Zahash Z. May I ask what is your expected behavior for the return value of __repr__() from PriorityQueue. I'm agreed with Stéphane Wirtel, I don't think returning all the items would be a good idea, maybe we can improve it in another way. For instance, return the first and last element as well as the length. |
|
|
| msg336062 - (view) |
Author: Stéphane Wirtel (matrixise) * |
Date: 2019-02-20 10:37 |
| >>> from collections import deque >>> d = deque() >>> d.append('j') >>> d.appendleft('f') >>> d deque(['f', 'j']) >>> repr(d) "deque(['f', 'j'])" Maybe there is a solution, in the code of deque_repr, we convert the deque to a list. We could do the same thing and take the first and the last element. |
|
|
| msg336066 - (view) |
Author: Stéphane Wirtel (matrixise) * |
Date: 2019-02-20 10:58 |
| For this script, you could have the following output: from queue import Queue q = Queue() print(repr(q)) q.put('a') print(repr(q)) q.put('b') print(repr(q)) q.put('c') print(repr(q)) Queue() Queue('a') Queue('a','b') Queue('a'...'c') |
|
|
| msg336067 - (view) |
Author: Stéphane Wirtel (matrixise) * |
Date: 2019-02-20 11:04 |
| New output for Queue, PriorityQueue and LifoQueue Queue() Queue('a') Queue('a','b') Queue('a'...'c') PriorityQueue() PriorityQueue('a') PriorityQueue('a','b') PriorityQueue('a'...'c') LifoQueue() LifoQueue('a') LifoQueue('a','b') LifoQueue('a'...'c') |
|
|
| msg336068 - (view) |
Author: Stéphane Wirtel (matrixise) * |
Date: 2019-02-20 11:09 |
| Just created a PR for this issue and I need to add the tests for the __repr__ method |
|
|
| msg336073 - (view) |
Author: Zahash Z (Zahash Z) * |
Date: 2019-02-20 11:31 |
| I raised this issue after creating a pull request and modifying the source code. so, people who want to create a pull request can sit back and relax cuz I got this |
|
|
| msg336075 - (view) |
Author: Rémi Lapeyre (remi.lapeyre) * |
Date: 2019-02-20 12:09 |
| Hi @matrixise, is it really an issue to iterate over all the elements of the deque? __repr__ won't be called on performance sensitive path and we already do this for repr([0]*10000), repr({i for i in range(10000)}) and repr({i:i for i in range(10000)}) |
|
|
| msg336076 - (view) |
Author: Stéphane Wirtel (matrixise) * |
Date: 2019-02-20 12:14 |
| Yep, we have a problem, two PRs for the same issue. I have seen your issue in the bug tracker (not marked as "in progress") and I started to fix it where I have proposed some solutions and a PR. For the next time, maybe you should create the issue, indicate you are working on the issue and fix it. I am really sorry, I didn't see that you was working on your issue because there was no associated PR on the issue. And now, I understand because the title of your PR was "issue #36049; added __repr__() for the queue.PriorityQueue and queue.LifoQueue classes". Bedevere (a BOT) tries to map a PR with an issue if the title is correctly defined in Github. For example: "bpo-36049: Added __repr__() for the queue.PriorityQueue and queue.LifoQueue classes" Have a nice day, |
|
|
| msg336077 - (view) |
Author: Stéphane Wirtel (matrixise) * |
Date: 2019-02-20 12:17 |
| @remi.lapeyre In fact, I am not sure about the performance issue but in the code of _collectionsmodule.c#deque_repr we create a PySequence_List, it's a new list. So for me, but maybe I am wrong, we will iterate over the deque container for the creation of the new list. now, I think it's not a big issue (to confirm by @rhettinger) |
|
|
| msg336078 - (view) |
Author: Stéphane Wirtel (matrixise) * |
Date: 2019-02-20 12:19 |
| static PyObject * deque_repr(PyObject *deque) { PyObject *aslist, *result; ... aslist = PySequence_List(deque); ... if (((dequeobject *)deque)->maxlen >= 0) result = PyUnicode_FromFormat("%s(%R, maxlen=%zd)", _PyType_Name(Py_TYPE(deque)), aslist, ((dequeobject *)deque)->maxlen); else result = PyUnicode_FromFormat("%s(%R)", _PyType_Name(Py_TYPE(deque)), aslist); ... return result; } |
|
|
| msg336085 - (view) |
Author: Rémi Lapeyre (remi.lapeyre) * |
Date: 2019-02-20 13:14 |
| Note: when repr do not round trip, I think it's common practice to use brackets: <LifoQueue('a'...'c')> instead of: LifoQueue('a'...'c') Example with Regex from https://docs.python.org/3/howto/regex.html: >>> re.match(r'From\s+', 'From amk Thu May 14 19:12:10 1998') <re.Match object; span=(0, 5), match='From '> |
|
|
| msg336086 - (view) |
Author: Stéphane Wirtel (matrixise) * |
Date: 2019-02-20 13:17 |
| Fixed |
|
|
| msg336087 - (view) |
Author: Rémi Lapeyre (remi.lapeyre) * |
Date: 2019-02-20 14:24 |
| See also https://bugs.python.org/issue35889 which add repr to sqlite3.Row |
|
|
| msg336114 - (view) |
Author: Serhiy Storchaka (serhiy.storchaka) * |
Date: 2019-02-20 16:47 |
| I guess you hurried to write the code. It has not yet been decided whether to expose the content of the queue in its repr at all. There is no public API for accessing the content of the queue without removing items from the queue, and I think it is intentional. What is your use case? Why you need to change the repr of queues? |
|
|
| msg336116 - (view) |
Author: Zahash Z (Zahash Z) * |
Date: 2019-02-20 16:52 |
| If you look at the queue.PriorityQueue class, there is self.queue = [ ]. That's because priority queue uses list data structure to implement the min heap data structure (on which priority queue is based on). So the list can be represented. There is no need to remove anything. On Wed, 20 Feb 2019, 10:17 pm Serhiy Storchaka, <report@bugs.python.org> wrote: > > Serhiy Storchaka <storchaka+cpython@gmail.com> added the comment: > > I guess you hurried to write the code. It has not yet been decided whether > to expose the content of the queue in its repr at all. There is no public > API for accessing the content of the queue without removing items from the > queue, and I think it is intentional. > > What is your use case? Why you need to change the repr of queues? > > ---------- > nosy: +serhiy.storchaka > > _______________________________________ > Python tracker <report@bugs.python.org> > <https://bugs.python.org/issue36049> > _______________________________________ > |
|
|
| msg336118 - (view) |
Author: Raymond Hettinger (rhettinger) * |
Date: 2019-02-20 16:56 |
| [Zahash Z] (Zahash Z)] > If you look at the queue.PriorityQueue class, there is > self.queue = [ ]. We really don't want to expose the internals here. They are subject to change and access to them is guarded by locks. > It has not yet been decided whether to expose the content > of the queue in its repr at all. There is no public API for > accessing the content of the queue without removing items > from the queue, and I think it is intentional. I agree with Serhiy. Let's close this feature request. It doesn't make sense for queues and is at odds with their design and intended uses. |
|
|
| msg336122 - (view) |
Author: Stéphane Wirtel (matrixise) * |
Date: 2019-02-20 17:07 |
| Hi @rhettinger Thank you for the review of this issue. |
|
|
| msg336125 - (view) |
Author: Serhiy Storchaka (serhiy.storchaka) * |
Date: 2019-02-20 17:10 |
| If you treat the "queue" attribute as a part the public API, just use repr(q.queue) instead of repr(q). |
|
|
| msg336128 - (view) |
Author: Raymond Hettinger (rhettinger) * |
Date: 2019-02-20 17:26 |
| > It has not yet been decided whether to expose the content > of the queue in its repr at all. There is no public API for > accessing the content of the queue without removing items > from the queue, and I think it is intentional. I agree with Serhiy. Let's close this feature request. It doesn't make sense for queues and is at odds with their design and intended uses. There are a number of places where making a less opaque repr is helpful, but iterators and queues have a temporal aspect where this doesn't make sense. |
|
|
| msg336145 - (view) |
Author: Stéphane Wirtel (matrixise) * |
Date: 2019-02-20 19:54 |
| Hi @rhettinger and @serhiy Really sorry, I did not see that I have re-opened the issue. |
|
|