FIX: make safe to add / remove artists during ArtistList iteration #22229
Conversation
If you remove artists during iteration some of the artists will be skipped due to the underlying list being mutated.
There was a problem hiding this comment.
But this seems to be normal Python semantics, no? ("if you want to iterate on a list and remove entries from it at the same time, you need to make a copy first") I'm not sure we should deviate from standard semantics here (especially as the name ArtistList does suggest list-like semantics.
Hence blocking as I think this at least needs some more discussion, but open to changing my mind.
There was a problem hiding this comment.
ArtistList exists to make the returned list finally immutable.
If we'll do this using a tuple, as suggested in the ArtistList docstring, that will effectively be equivalent to the behavioral change in this PR. So anticipating the change already is fine.
One could imagine alternative scenarios with a read-only proxy object to the internal data, which would suffer the modification problem. Such an object would only make sense if creating a copy is too expensive, but I don't think that's the case.
I had the same thought, however because it is a proxy which is providing a list-like interface of a filtered view on top of another list I think we should err on the side of hiding the mutation. Because all of the proxy lists operate on top of the same underlying list you could come up with a pathological example where you are iterating over and mutating more than one and they start to affect each other. |
|
Even immutable proxies on mutable containers don't copy the entries, e.g. |
|
Moved to 3.6 and I'll add a behavior change note? |
c3ae176 to
c6b0f25
Compare
Co-authored-by: Elliott Sales de Andrade <quantum.analyst@gmail.com>
PR Summary
If you remove artists during iteration some of the artists will be skipped due
to the underlying list being mutated.
PR Checklist
Tests and Styling
pytestpasses).flake8-docstringsand runflake8 --docstring-convention=all).