On 23 November 2017 at 15:22, bunslow <bunslow@gmail.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)

and:

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.

Cheers,

Nick.

Nick Coghlan | ncoghlan@gmail.com | Brisbane, Australia