yeah - github picks up your changes on the branch automatically. we'll work on reviewing.
On Fri, Jan 27, 2017 at 7:11 PM, Paul A. Jackson <paul.jack...@pb.com> wrote: > I am git illiterate. > I committed to my repo. > It looks like that's all I have to do. > Travis build tells me to hang tight. > It knows the commit is related to an open pull request > and your repo knows to pull it in automatically? > That's cool, if true. > -Paul > > -----Original Message----- > From: Paul A. Jackson [mailto:paul.jack...@pb.com] > Sent: Friday, January 27, 2017 2:24 PM > To: dev@tinkerpop.apache.org > Subject: RE: [GitHub] tinkerpop issue #548: TINKERPOP-1589 Re-introduced > CloseableIterator > > Hi Stephen, > > I think there are two parts to this, and I may have responded to the wrong > part. > > I can easily and cleanly make those subclasses implement AutoCloseable and > remove that from FlatMapStep. > > Implementing AutoCloseable is needed for the second half of the problem, > where a traversal is closed before it is read to completion. This is easily > delegated to the subclasses. > > There's the first half of the problem dealing with closing iterators as > FlatMapStep finishes with them, but before it loses its reference to them. > This is very hard to remove from FlatMapStep without duplicating all the > code in the subclasses. > > If you consent I'd like to update the pull request that removes > AutoCloseable from FlatMapStep but keeps the closing of iterators on the > fly in FlatMapStep. > > I'd can also move that try/catch stuff to a helper method (in > CloseableInterface?). That keeps FlatMapStep very clean. > > Thoughts? > > public abstract class FlatMapStep<S, E> extends AbstractStep<S, E> { > > private Traverser.Admin<S> head = null; > protected Iterator<E> iterator = EmptyIterator.instance(); // Made > protected > > public FlatMapStep(final Traversal.Admin traversal) { > super(traversal); > } > > @Override > protected Traverser.Admin<E> processNextStart() { > while (true) { > if (this.iterator.hasNext()) { > return this.head.split(this.iterator.next(), this); > } else { > this.head = this.starts.next(); > closeIterator(); // Added > this.iterator = this.flatMap(this.head); > } > } > } > > protected abstract Iterator<E> flatMap(final Traverser.Admin<S> > traverser); > > @Override > public void reset() { > super.reset(); > closeIterator(); // Added > this.iterator = EmptyIterator.instance(); > } > > void closeIterator() { > CloseableIterator.closeIterator(this.iterator); > } > } > > Thanks, > -Paul > > -----Original Message----- > From: Paul A. Jackson [mailto:paul.jack...@pb.com] > Sent: Friday, January 27, 2017 1:55 PM > To: dev@tinkerpop.apache.org > Subject: RE: [GitHub] tinkerpop issue #548: TINKERPOP-1589 Re-introduced > CloseableIterator > > I can do this, and I can see why you are suggesting it, but I didn't > originally because it means overriding all the methods from FlatMapStep > into these subclasses. > > The way it's currently designed all the subclasses need to do is implement > flatMap(). FlatMapStep has private access to iterator and head. It knows > when it is done with an iterator, that when it's done with an iterator that > it should replace it with an EmptyIterator, that if it's not done it needs > to call head.split with the next element, etc. > > To implement what you suggest, correct me if I'm wrong, all that private > knowledge needs to be duplicated in the subclasses. If feels like a bad > design because if any of those behaviors change in FlatMapStep we need to > remember to go into the subclasses and mirror the changes there. > > Maybe I should move the try/catch stuff to a helper method so that all > FlatMapStep needs to do is execute one line when it's done with an iterator? > > Should I submit parallel pull request so you can compare/contrast? > > -Paul > > -----Original Message----- > From: spmallette [mailto:g...@git.apache.org] > Sent: Friday, January 27, 2017 1:23 PM > To: dev@tinkerpop.apache.org > Subject: [GitHub] tinkerpop issue #548: TINKERPOP-1589 Re-introduced > CloseableIterator > > Github user spmallette commented on the issue: > > https://github.com/apache/tinkerpop/pull/548 > > I'm pretty sure @okram was affirming that `VertexStep`, > `EdgeVertexStep` and `PropertiesStep` should implement `AutoCloseable` (I > don't think that there are others) rather than a blanket change of just > applying it to `FlatMapStep`. As the logic is re-used, perhaps you could do > an `ElementStep` that extends the `FlatMapStep` and implements > `AutoCloseable`. Then have those three steps extend `ElementStep` which > would contain the close logic that you have. > > > --- > If your project is set up for it, you can reply to this email and have > your reply appear on GitHub as well. If your project does not have this > feature enabled and wishes so, or if the feature is enabled but not > working, please contact infrastructure at infrastruct...@apache.org or > file a JIRA ticket with INFRA. > --- > > ________________________________ > > > ________________________________ > > > ________________________________ > >
