[GitHub] jena pull request #323: JENA-1427: ExtendedIterator.nextOptional

[afs <gi...@git.apache.org>]

GitHub user afs opened a pull request:

    https://github.com/apache/jena/pull/323

    JENA-1427: ExtendedIterator.nextOptional

    

You can merge this pull request into a Git repository by running:

    $ git pull https://github.com/afs/jena next-optional

Alternatively you can review and apply these changes as the patch at:

    https://github.com/apache/jena/pull/323.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

    This closes #323
    
----
commit 1a8e270fc48b5b5b3eefe24314fb75174723ab43
Author: Andy Seaborne <an...@apache.org>
Date:   2017-11-30T10:46:41Z

    JENA-1427: ExtendedIterator.nextOptional

----


---

[GitHub] jena issue #323: JENA-1427: ExtendedIterator.nextOptional

[afs <gi...@git.apache.org>]

Github user afs commented on the issue:

    https://github.com/apache/jena/pull/323
  
    That was already the case in the PR and I've added text to call it out explicitly into the javadoc.


---

[GitHub] jena issue #323: JENA-1427: ExtendedIterator.nextOptional

[ajs6f <gi...@git.apache.org>]

Github user ajs6f commented on the issue:

    https://github.com/apache/jena/pull/323
  
    Okay, that works for me!


---

[GitHub] jena issue #323: JENA-1427: ExtendedIterator.nextOptional

[afs <gi...@git.apache.org>]

Github user afs commented on the issue:

    https://github.com/apache/jena/pull/323
  
    Just the `nextOptional` part for JENA-1427.
    
    Adding the other methods discussed there continues.



---

[GitHub] jena issue #323: JENA-1427: ExtendedIterator.nextOptional

[ajs6f <gi...@git.apache.org>]

Github user ajs6f commented on the issue:

    https://github.com/apache/jena/pull/323
  
    Do we want to return `empty` for `null`? I ask because the "ergonomics" are different. The way we have it now:
    ```
    try {
        iterator.nextOptional().ifPresent(this::doStuff);
    }
    catch (NullPointerException e) { worry about that; }
    ```
    vs. just `iterator.nextOptional().ifPresent(this::doStuff);` where `doStuff` is responsible for testing for `null`. I actually like the latter because `null` might be a legitimate value, but I may be misunderstanding the contract of `ExtendedIterator`-- is it guaranteed that `next()` never returns `null`? Is `null` actually not ever legit?



---

[GitHub] jena issue #323: JENA-1427: ExtendedIterator.nextOptional

[afs <gi...@git.apache.org>]

Github user afs commented on the issue:

    https://github.com/apache/jena/pull/323
  
    This does not make sense to me - how can one tell from `nextOptional().orElseThrow()` when at the end without having to drop out of fluent? (or functional `first` is fluent in a functional sense).
    
    That's why I banned nulls from iterators when using `nextOptional`; I don't know of a case where `ExtendedIterator` can return nulls when used in the model API. 
    
    If `nextOptional` can have two different uses, we would be better off with `nextOr*` which do differentiate the two cases.



---

[GitHub] jena issue #323: JENA-1427: ExtendedIterator.nextOptional

[ajs6f <gi...@git.apache.org>]

Github user ajs6f commented on the issue:

    https://github.com/apache/jena/pull/323
  
    That's my point-- I don't think you can do a good fluent API for this without narrowing the `Iterator` contract to exclude `null`s, because you can't distinguish. We can do that (narrow the contact)-- we own `ExtendedIterator` and as you say, we aren't pushing `null`s through it for the `Model` API.
    
    So my vote is that we do what you have here, but _also_ add a note to `ExtendedIterator` guaranteeing that `next()` must not return `null`.


---

[GitHub] jena issue #323: JENA-1427: ExtendedIterator.nextOptional

[ajs6f <gi...@git.apache.org>]

Github user ajs6f commented on the issue:

    https://github.com/apache/jena/pull/323
  
    I'm saying we don't do `orElseThrow` or any other `orElseX`, we leave that to `nextOptional().orElseThrow()`, etc. Then at iterator end, `nextOptional()` returns `empty` forever. If there are `null`s, it returns `empty`. If you want to know whether that is because there are many `null`s or because the iterator is done, you call `hasNext()`.
    
    Another way to put it: we shouldn't try to eliminate needing `hasNext()` with this-- if we want to do that, we should do something like `Spliterator::tryAdvance`, which is a whole different story. You can se how that would work a little with `Iterator::forEachRemaining`. You don't need `hasNext()` with that.


---

[GitHub] jena pull request #323: JENA-1427: ExtendedIterator.nextOptional

[asfgit <gi...@git.apache.org>]

Github user asfgit closed the pull request at:

    https://github.com/apache/jena/pull/323


---

[GitHub] jena issue #323: JENA-1427: ExtendedIterator.nextOptional

[afs <gi...@git.apache.org>]

Github user afs commented on the issue:

    https://github.com/apache/jena/pull/323
  
    This seems to be arguing for NoSuchElementException for end of iterator and Optional.empty for null?
    
    (There needs to be a way to distinguish end-of-iterator from iterator-returns-null.)
    



---