Alternative implementation

[ericfrederich]

Did this as an exercise to see if it would look cleaner to implement this using collections.OrderedDict.

Use multiple inheritance (Sequence in addition to MutableSet) to get indexing.

This implementation doesn't have to keep track of two different objects self.items and self.map (though maybe collections.OrderedDict might be doing that internally)... either way, the code looks cleaner. Look at the implementation of the abstract methods.

The index method has this feature where you can pass it an iterable. Was able to implement this feature without having to re-implement index... we can just call super() and use the one we get from collections.Sequence

All tests pass without modifying test.py

[Erotemic]

Does this improve performance? Even if it doesn't it might be useful to have a nice set of benchmarks to test if any of the implementation internals can be improved.

If you are looking for a tool to measure runtime of code snippets I would recommend using my timerit module. I find its a bit simpler to use than the builtin timeit

[rspeer]

It appears to me that this is going to have a significantly negative impact on performance.

OrderedSet.index is a method that I frequently use on large sets. It's as fast as a dictionary lookup; effectively O(1). But this PR removes the implementation of it, and falls back on list.index, an inefficient O(N) operation.

[ericfrederich]

Yeah, I saw that it had a performance hit this morning when I looked at it. I'll see if there is an easy fix tonight.

[ericfrederich]

After new refactor commits there's a lot of conflicts. Also, the new code is inheriting from the same base classes I chose anyway. Closing this.

[rspeer]

Sounds good.

I can definitely see the appeal of a simpler OrderedSet that acts like OrderedDict, which is what I believe your implementation was. It's just that this OrderedSet has accumulated use cases over the years that rely on it working differently from OrderedDict, such as having fast indexed lookups.