Smalltalk › Squeak › Squeak - Dev

PositionableStream >> #peekBack behavior

_‹ Previous Topic Next Topic _›

Classic

List

Threaded

7 messages Options

darth-cheney

PositionableStream >> #peekBack behavior

Hello,

What is the expected behavior for a stream that is at position 1 when you send #peekBack? Intuitively I'd expect to get the first element of the underlying collection as a response. But instead I am getting nil.

Here is an example that is failing:

```

elem := #(1 2 3 4).
stream := ReadStream on: elem.
first := stream next.
first = 1.
back := stream peekBack.
back = first.

```

The last message is currently responding false. In Pharo this returns true, so there are differences.

The currently implementation of PositionableStream >> #peekBack is:

```

peekBack
"Return the element at the previous position, without changing position. Use indirect messages in case self is a StandardFileStream."

| element |
self position = 0 ifTrue: [self errorCantGoBack].
self position = 1 ifTrue: [self position: 0. ^ nil].
self skip: -2.
element := self next.
self skip: 1.
^ element

```

I can see the nil being returned there explicitly, so that's "where it's happening." Should this be the case though?

Thanks

Eric

Levente Uzonyi

Re: PositionableStream >> #peekBack behavior

Hi Eric,

On Thu, 6 Aug 2020, Eric Gade wrote:

> Hello,
>
> What is the expected behavior for a stream that is at position 1 when you send #peekBack? Intuitively I'd expect to get the first element of the underlying collection as a response. But instead I am getting nil.
>
> Here is an example that is failing:
> ```
> elem := #(1 2 3 4).
> stream := ReadStream on: elem.
> first := stream next.
> first = 1.
> back := stream peekBack.
> back = first.
> ```
> The last message is currently responding false. In Pharo this returns true, so there are differences.
>
> The currently implementation of PositionableStream >> #peekBack is:
> ```
> peekBack
> "Return the element at the previous position, without changing position. Use indirect messages in case self is a StandardFileStream."
>
> | element |
> self position = 0 ifTrue: [self errorCantGoBack].
> self position = 1 ifTrue: [self position: 0. ^ nil].
> self skip: -2.
> element := self next.
> self skip: 1.
> ^ element
> ```
>
> I can see the nil being returned there explicitly, so that's "where it's happening." Should this be the case though?

The previous, probably original implmenetation of #peekBack used to send
#oldBack to the stream.
IIRC #oldBack was based on the idea that the position of a stream is an
index of a sequence where #next means +1 to that index while #back means
-1 to that index. Following that logic, you have to #skip: -2 and send
#next to get the element -1 to position.

#oldBack has been removed but the behavior of #peekBack is presumably the
same as it was before. Some ancient but now external code may rely on
#peekBack but it's not very likely such code would work in the current
Trunk.
#peekBack has no real users in the Trunk only a test remembers what it
used to do.
So, I think it's a good time to change its behavior to be based on #back.

Levente

>
> Thanks
>
>
> --
> Eric
>
>

darth-cheney

Re: PositionableStream >> #peekBack behavior

On Thu, Aug 6, 2020 at 7:45 PM Levente Uzonyi <[hidden email]> wrote:

So, I think it's a good time to change its behavior to be based on #back.

Levente

Ok I have the change (along with a few new tests) made and ready to go. I have never contributed to Squeak before. I am trying to follow these instructions from the wiki.

I don't have a lot of experience here with Monticello and changesets, however. Right now my "changes" are across two different packages (Streams and Tests-Streams). Do I "save" these commits separately, or is there some way to have them both be in one commit?

Thanks.

Eric

Levente Uzonyi

Re: PositionableStream >> #peekBack behavior

Hi Eric,

On Fri, 7 Aug 2020, Eric Gade wrote:

>
>
> On Thu, Aug 6, 2020 at 7:45 PM Levente Uzonyi <[hidden email]> wrote:
>
> So, I think it's a good time to change its behavior to be based on #back.
>
>
> Levente
>
>
> Ok I have the change (along with a few new tests) made and ready to go. I have never contributed to Squeak before. I am trying to follow these instructions from the wiki.
> I don't have a lot of experience here with Monticello and changesets, however. Right now my "changes" are across two different packages (Streams and Tests-Streams). Do I "save" these commits separately, or is there some way
> to have them both be in one commit?

It should be two commits, since commits are per package in Monticello.
But, we don't have any packages named Streams or Test-Streams in Squeak.
ReadStream is in the Collections package while ReadStreamTest is in the
CollectionsTests package.
Are you sure Streams and Tests-Streams are the names of the packages in
your image?

Levente

>
> Thanks.
> --
> Eric
>
>

David T. Lewis

Re: PositionableStream >> #peekBack behavior

On Sat, Aug 08, 2020 at 01:52:17AM +0200, Levente Uzonyi wrote:

> Hi Eric,
>
> On Fri, 7 Aug 2020, Eric Gade wrote:
>
> >
> >
> >On Thu, Aug 6, 2020 at 7:45 PM Levente Uzonyi <[hidden email]> wrote:
> >??
> > So, I think it's a good time to change its behavior to be based on
> > #back.
> >
> >
> > Levente
> >
> >
> >Ok I have the change (along with a few new tests) made and ready to go. I
> >have never contributed to Squeak before. I am trying to follow these
> >instructions from the wiki.
> >I don't have a lot of experience here with Monticello and changesets,
> >however. Right now my "changes" are across two different packages (Streams
> >and Tests-Streams). Do I "save" these commits separately, or is there some
> >way
> >to have them both be in one commit?
>
> It should be two commits, since commits are per package in Monticello.
> But, we don't have any packages named Streams or Test-Streams in Squeak.
> ReadStream is in the Collections package while ReadStreamTest is in the
> CollectionsTests package.
> Are you sure Streams and Tests-Streams are the names of the packages in
> your image?
>

Hi Eric,

To add to what Levente says, Usually what you would want to do is
save your changes using an existing package. For example, if you have
some changes to class PositionableStream, that class is part of the
'Collections-Streams' category in a system browser, and when you look
at the packages in a Monticello browser, this category is part of the
'Collections' package (this is all just based on naming conventions).

If your image is up to date with the trunk update stream, then your
image will be using Collections-eem.907.mcz from the trunk repository,
and the changes that you have made in your image would be relative to
that MCZ version. When you save your changes to the inbox, you would
be saving a new version called (probably) collections-EG.908.mcz.

You will save your tests separately, probably in the 'CollectionsTests'
package. It is fine to have the two separate, although sometimes it
is helpful to add something in the commit comments along the lines of
"these tests go with those changes" or "these updates in Collections
make the tests in CollectionsTests pass" just to make the connection.

Don't worry about getting this perfect (especially the first time!).
Even if there is a mistake it is usually easy for people to figure out
what the changes were and merge them at a later time.

HTH,
Dave

Chris Muller-3

Re: PositionableStream >> #peekBack behavior

In reply to this post by Levente Uzonyi

> I can see the nil being returned there explicitly, so that's "where it's happening." Should this be the case though?

The previous, probably original implmenetation of #peekBack used to send
#oldBack to the stream.
IIRC #oldBack was based on the idea that the position of a stream is an
index of a sequence where #next means +1 to that index while #back means
-1 to that index. Following that logic, you have to #skip: -2 and send
#next to get the element -1 to position.

#oldBack has been removed but the behavior of #peekBack is presumably the
same as it was before. Some ancient but now external code may rely on
#peekBack but it's not very likely such code would work in the current
Trunk.
#peekBack has no real users in the Trunk only a test remembers what it
used to do.

Given the above, and given that we have #next:, I ended up balancing that API with #peek:, and that again with #peekBack:. Since they all simply return a String, possibly empty, they dodge the question about nil vs. error entirely.

So, I think it's a good time to change its behavior to be based on #back.

Without wanting to sound ungrateful for Eric's contribution (of which I look forward to more of), may we also consider the addition-by-subtraction opportunity? I mean, it kinda makes sense that there wouldn't, and won't, be any users of #peekBack. Maybe we should deprecate it.

- Chris

David T. Lewis

Re: PositionableStream >> #peekBack behavior

On Sat, Aug 08, 2020 at 04:21:27PM -0500, Chris Muller wrote:

> >
> > > I can see the nil being returned there explicitly, so that's "where it's
> > happening." Should this be the case though?
> >
> > The previous, probably original implmenetation of #peekBack used to send
> > #oldBack to the stream.
> > IIRC #oldBack was based on the idea that the position of a stream is an
> > index of a sequence where #next means +1 to that index while #back means
> > -1 to that index. Following that logic, you have to #skip: -2 and send
> > #next to get the element -1 to position.
> >
> > #oldBack has been removed but the behavior of #peekBack is presumably the
> > same as it was before. Some ancient but now external code may rely on
> > #peekBack but it's not very likely such code would work in the current
> > Trunk.
> > #peekBack has no real users in the Trunk only a test remembers what it
> > used to do.
> >
>
> Given the above, and given that we have #next:, I ended up balancing that
> API with #peek:, and that again with #peekBack:. Since they all simply
> return a String, possibly empty, they dodge the question about nil vs.
> error entirely.
>
> So, I think it's a good time to change its behavior to be based on #back.
> >
>
> Without wanting to sound ungrateful for Eric's contribution (of which I
> look forward to more of), may we also consider the addition-by-subtraction
> opportunity? I mean, it kinda makes sense that there wouldn't, and won't,
> be any users of #peekBack. Maybe we should deprecate it.
>

That's probably the best idea of all.

As far as I can tell, #peekBack was used in PositionableStream>>#backChunk
in the Squeak 3.8 era, but Levente's 3/22/2010 version of the method in
trunk today eliminated the need for it.

So given that nobody uses it and we don't know how it should work, +1 for deprecation.

And +1 to looking forward to more contributions from Eric :-)

Dave