Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SortedList is not actually a MutableSequence #138

Open
pganssle opened this issue Jan 17, 2020 · 1 comment · May be fixed by #139
Open

SortedList is not actually a MutableSequence #138

pganssle opened this issue Jan 17, 2020 · 1 comment · May be fixed by #139

Comments

@pganssle
Copy link

My understanding is that SortedList.extend was removed because there was some sort of efficiency concern, and it was deemed preferable for people to use SortedList.update. Similarly, SortedList.append throws an error and it is preferable for people to use add.

I guess the reasoning here is that append and extend are not actually meaningful concepts to a SortedList (since "append" means "add to the end" not "add in sorted order).

However, throwing NotImpementedError breaks Liskov substitution for SortedList - it's not actually a MutableSequence, since a MutableSequence is guaranteed to have extend and append methods. This is a problem because code that uses type hinting to accept MutableSequence, or dispatches based on type is expecting to be able to use these methods.

I think it would be best to drop MutableSequence as a base class in favor of Sequence and make sure that these methods are actually not implemented (either using the __getattr__ trick used in SortedDict or just by dropping them entirely) so that isinstance checks will properly fail.

An alternative would be to accept a lesser Liskov substitution violation and make them aliases for update and add - possibly with an associated warning, but I think I do agree with the decision to drop them.

@pganssle pganssle linked a pull request Jan 17, 2020 that will close this issue
@grantjenks grantjenks added the V3 label Jun 7, 2020
@grantjenks grantjenks removed the V3 label Aug 20, 2022
@AndreiPashkin
Copy link

AndreiPashkin commented May 22, 2023

I think that append() could be useful to add an element which is larger than current maximum to the end or throw a some kind of error.

upd.
Something like that has already been suggested in #178

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants