Skip to content

Conversation

@greateggsgreg
Copy link
Contributor

This PR is a companion to #50, since issubclass() and isinstance() should imply that all of the ABC's methods are implemented.

With the exception of index() and __contains__(), the methods added to the persistent classes require reading the entire object anyway, so I just use self.read_all(). As I was posting this, I realized that __contains__() can be chained, not requiring the entire object to be read. I will modify that method when I revisit this.

The methods added to the transient classes follow the pattern of using as little memory as possible, but in the case of __reversed__(), I didn't see a way to avoid having the entire object in memory at first.

The index() method is mostly implemented in the list base class, with the child methods simply handling the offset math.

I do have tests, but they need refactoring before I push them. I will be out of town until next Monday, but I wanted to push what I had to see if you had any initial feedback.

* Still need __len__ and __contains__ for transient list
* Still need __ne__ for transient object
@daggaz
Copy link
Owner

daggaz commented Jul 19, 2023

Hmm, interesting point on transient __reversed__(), I'm tempted to say we should just raise NotImplementedError, perhaps with a helpful message indicating that you should switch to persistent types for this part or first convert to standard types, or something?

Maybe a new section in the docs with the exception linking to there?

return f"<{type(self).__name__}: {repr(self._data)}, {'STREAMING' if self.streaming else 'DONE'}>"

def __contains__(self, item):
self.read_all()
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could be implemented so that, if item is near the start, you don't have to read the whole thing.

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 this pull request may close these issues.

2 participants