REF: make coordinates not a state variable in io.pytables #29805
Conversation
| @@ -1644,14 +1643,12 @@ def __init__( | |||
|
|
|||
| self.auto_close = auto_close | |||
|
|
|||
| def __iter__(self): | |||
There was a problem hiding this comment.
Hmm on board with the goal, but is this really what we want to do? Wouldn't this mean that the TableIterator class is actually no longer an iterator by definition?
There was a problem hiding this comment.
I guess. In the case that doesn't go through 1674-1675 below TableIterator is already not really an iterator, so in that sense this makes the name consistently inaccuraet
There was a problem hiding this comment.
Ha sounds good. I'll defer to others more familiar with this code
| @@ -1644,14 +1643,12 @@ def __init__( | |||
|
|
|||
| self.auto_close = auto_close | |||
|
|
|||
| def __iter__(self): | |||
There was a problem hiding this comment.
so there is an open issue to have this just subclass BaseIterator which should preserve the correct iterator behavior. I would rather fix it that way.
There was a problem hiding this comment.
I don't see the issue you're referring to, but before long I'll do a dedicated look through the HDF5 issues to see what can be closed.
It isn't clear that subclassing BaseIterator would affect the statefulness that this PR is addressing
There was a problem hiding this comment.
umm, not those. but in any event i think if you subclass BaseIterator this will just work
There was a problem hiding this comment.
do you mean collections.abc.Iterator? that would mean we'd have to define __next__ and __iter__ would return self. But for __next__ to work, it the relevant coordinates variable would have to be stateful, which is exactly what this PR is try to avoid
There was a problem hiding this comment.
this is a non-standard way of doing things, it needs to inheirt from https://github.com/pandas-dev/pandas/blob/master/pandas/io/common.py#L81
There was a problem hiding this comment.
Thanks for the link.
If I'm reading you right, you are adamant that TableIterator itself should be an iterator. Is that accurate?
| indirect = next(iterator.__iter__()) | ||
| tm.assert_frame_equal(direct, indirect) | ||
| iterator.store.close() |
There was a problem hiding this comment.
I think its a problem not closing the iterator; the fact that you had to change this is suspicious
There was a problem hiding this comment.
iter_result does close the store
|
This particular statefulness is turning out to not be that bothersome. Closing. |
orthogonal to #29804.