On 23 November 2017 at 15:22, bunslow email@example.com wrote:
Something *should* be object oriented with the functions in question all operate on the same data type, and in particular, those functions/verbs *are only well defined for that type*. heapq.heappush(list-not-heap, item) is perfectly valid code in the current interface, but doesn't make any sense at all. And list.sort is *not* function based, it's an object oriented method. sorted() provides the same functionality for other types, given that it's a well defined function for a wide variety of sequences (unlike heappush). (list.sort is a method because unlike sorted(), it operates inplace, and is thus only meaningful for mutable, "complete" (all in memory, not "lazy") sequences -- i.e., a list.)
I've never used bisect, so I'll refrain from commenting on it.
At the end of the day, the patch proposed is merely a wrapper around the functional approach; you are welcome to continue using it as you like, it's not going anywhere. I would propose that the docs put the OOP version first though.
Ensuring the docs are clearly separated is part of why I think "collections.Heap" would make more sense as a proposal than "heapq.Heap" - collections already imports heapq for use in the Counter implementation, and adding another primitive container type to collections is a smaller conceptual change than updating heapq to being more than just a heap algorithm library.
I'd also note that a collections.Heap class that accepts the underlying list to use as its sole parameter would compose nicely with the list builtin to allow creation of a heap from an arbitrary iterable:
heap = collections.Heap(list(iterable))
rather than the current:
heap = list(iterable) heapq.heapify(heap)
That's akin to the difference between:
data = sorted(iterable)
data = list(iterable) data.sort()
I don't think it would be a disaster if we continued to leave this out, but I don't think it would be a major maintenance burden or future barrier to learning to add it, either.