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
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. ^ elementI 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
On Thu, Aug 6, 2020 at 7:45 PM Levente Uzonyi leves@caesar.elte.hu 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 https://wiki.squeak.org/squeak/3279. 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.
Hi Eric,
On Fri, 7 Aug 2020, Eric Gade wrote:
On Thu, Aug 6, 2020 at 7:45 PM Levente Uzonyi leves@caesar.elte.hu wrote: So, I think it's a good time to change its behavior to be based on #back.
LeventeOk 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
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 leves@caesar.elte.hu wrote: ?? So, I think it's a good time to change its behavior to be based on #back.
LeventeOk 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
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
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
Finally merged via Collections-mt.1009 and CollectionsTests-mt.376.
The #old* interface is not yet deprecated but tested.
Best, Marcel Am 09.08.2020 00:10:20 schrieb David T. Lewis lewis@mail.msen.com: 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
squeak-dev@lists.squeakfoundation.org
-
Chris Muller -
David T. Lewis -
Eric Gade -
Levente Uzonyi -
Marcel Taeumel