Also I'd like to say I'm sorry if I failed to follow any standards or procedures either here or on the issues site. This is my first time attempting to contribute so I'm rather unsure of myself.

Well my first attempt was naive, and I was missing an obvious test for whether or not this would perform at the end of an Iterator. I am working on a more complete approach currently.

Well this turns out to do the exact same validation check as before however it appropriately works in the test cases as I didn't realize I was advancing the iterator past where I was evaluating. Hopefully this is seen as the best of both worlds as the implementation is left up to the inheritor.

Hi, iterators are deceptively tricky. You're in good hands with @szeiger .

You probably didn't intend this null handling:

scala> val b = List("hi", null, "world").iterator.buffered
b: scala.collection.BufferedIterator[String] = non-empty iterator

scala> if (b.nonEmpty) Option(b.head) else None
res0: Option[String] = Some(hi)

scala> b.next
res2: String = hi

scala> if (b.nonEmpty) Option(b.head) else None
res3: Option[String] = None                                      // Some(null)

scala> b.next
res4: String = null

scala> if (b.nonEmpty) Option(b.head) else None
res5: Option[String] = Some(world)

I don't know whether they'll want to add convenience API (which can be coded as extension methods). But I notice there's no way to ask a BufferedIterator whether the buffer is nonEmpty. I might want to process the next element, but only if it's buffered (perhaps because next incurs I/O). Not sure what you'd call it. Maybe def flush(): Option[A] which would also consume the element. Or nextBuffered. That would be a question for gitter.

I don't understand the argument for calling nonEmpty instead of hasNext. nonEmpty is !isEmpty and isEmpty is !hasNext, so this only adds two layers of indirection with a chance to hit an inappropriately overwritten method. I think it's a safe assumption that all BufferedIterator implementations have an efficient hasNext. I can't imagine how you would implement it differently or why you would want to have an inefficient hasNext and then override isEmpty to be more efficient.

Agreed on the null handling. An Iterator may legitimately contain null, so you need to use Some(head) instead of Option(head). This would make a good test case.

@som-snytt I don't see how you could retrofit a method that checks whether the buffer is non-empty into BufferedIterator. It would require a new method that has to be implemented by all subclasses. I'm not convinced how useful it would be, either. You never really care about an abstract concept such as "has a buffered element". You care about "performs I/O" or "holds on to a large chunk of memory" but those are more specific effects that you wouldn't want to deal with in the collection library. This could be done in a subtype of BufferedIterator in an I/O library.

I agree with you @szeiger . The options available don't really seem to make much sense. It belongs in GenTraversableLike, as it is. I tried hasNext realized a shortcoming at the end value of some Iterable implementations decided to use nonEmpty and did not realize that it was !hasNext until after I had posted the commit.

The problem is as posted the iterator makes no guarantees as to the buffer, nor is it ever truly safe to do so. The buffered iterator is free to advance past the end of the stream. As it advances the Iterator and stores the value, however if no value exists we receive the error. We see this in the buffered function. It is doubly unnecessary as it can become a Transversable via the toStream method which would offer this functionality.

Overall I am glad to have taken a shot at this, and learned a bit in the process but I am fairly certain that this not really the place. The buffer is unstable in both of the locations it is implemented. And traversable seems to make more sense, and it comforts me that my chosen implementation seems to mirrored it. Although I seem to have a lot more to learn about null in scala as my daily life in scala is so devoid of it.

Thank you for your time and I will continue to try to help out. Was my overall approach acceptable? Anything I should do differently on another issue?

What problems does hasNext have? I can't see anything wrong with if(hasNext) Some(head) else None. Do you have a test where it behaves incorrectly?

Well I believed incorrectly that I could implement a version where it would advance past the end of the list. Which is obviously untrue and a sign of my inexperience in this realm. I have added the test for null handling, and the code so that it uses hasNext.

I think this implementation should be fine. The new method still needs a scaladoc comment.

Testing Iterators is always tricky because of the (possibly intricate) mutable state. All your tests use a fresh Iterator.buffered implementation in its initial state. You may want to test that the behavior is also correct after hasNext and after next, and that calls to head and headOption are idempotent. For example, for a non-empty BufferedIterator, the following should hold:

val v1 = it.head
val v2 = it.headOption
val v3 = it.head
val v4 = it.headOption
v1 == v3 && v2 == v4 && Some(v1) == v2

Another consideration is binary compatibility. Unlike most projects in the Scala ecosystem which only guarantee backward compatibility between minor releases, Scala itself also guarantees forward compatibility. This means we cannot add new non-private methods in 2.12.x, so you need to target the 2.12.0 branch.

I have made the changes requested. I am uncertain what you mean about targeting for 2.12.0 vs 2.12.x, although I do understand the reason. Do I need to do something differently?

You can see the target branch at the top of this PR. You selected it when opening the PR (or not, because 2.12.x is the current default). There's no way to change it after the fact so you need to close this PR and open a new one against 2.12.0. While you're at it, please squash all changes into a single commit.

Oh, sorry, I just noticed that we haven't branched yet, so 2.12.x is indeed the correct branch.

Closing for New Squashed Commit

Just FYI, you don't need to create a new PR when you squash. Just do it in the original branch and git push -f it.