Test and Patch for writeStream error

Hi,

I stumbled on an error in WriteStream>>ensureEndsWith: method. The error
caused extra blank lines in logs.

Attached small changeset fixes it. It also adds a test to catch such
errors in the future. With this fix,

./pharo generator.image PharoVMSpur32Builder buildUnix32

gives a compact output without extra blank lines.

To which box in which repo should I save this cs for mainlining it?

Regards .. Subbu

Hi,

Thanks for reporting!

If you want to contribute and have your fix integrated, I advise you to follow the procedure here: http://pharo.org/contribute-propose-fix

If you need help, don't hesitate to reply!

Regards,
Vincent

!!!*************************************************************************************
"Ce message et les pièces jointes sont confidentiels et réservés à l'usage exclusif de ses destinataires. Il peut également être protégé par le secret professionnel. Si vous recevez ce message par erreur, merci d'en avertir immédiatement l'expéditeur et de le détruire. L'intégrité du message ne pouvant être assurée sur Internet, la responsabilité de Worldline ne pourra être recherchée quant au contenu de ce message. Bien que les meilleurs efforts soient faits pour maintenir cette transmission exempte de tout virus, l'expéditeur ne donne aucune garantie à cet égard et sa responsabilité ne saurait être recherchée pour tout dommage résultant d'un virus transmis.

This e-mail and the documents attached are confidential and intended solely for the addressee; it may also be privileged. If you receive this e-mail in error, please notify the sender immediately and destroy it. As its integrity cannot be secured on the Internet, the Worldline liability cannot be triggered for the message content. Although the sender endeavours to maintain a computer virus-free network, the sender does not warrant that this transmission is virus-free and will not be liable for any damages resulting from any virus transmitted.!!!"

Hi Subbu,

On Tue, Apr 18, 2017 at 09:04:00PM +0530, K K Subbu wrote:
This appears to be changing the meaning of #ensureEndsWith:.

The previous meaning was that it always ends with the supplied object.

You've modified it to mean that an empty collection doesn't have the
object appended.

Is that really what the method is supposed to mean?  The comment matched
the previous implementation, with your change, it doesn't.

If the change does go ahead, the comment should be updated to reflect
the changed definition.

I'm also wondering why you've changed the test to hard-code a string
stream?

Cheers,
Alistair

On Tuesday 18 April 2017 09:51 PM, Alistair Grant wrote:
> This appears to be changing the meaning of #ensureEndsWith:.
> The previous meaning was that it always ends with the supplied object.

The method name is ambiguous. If the collection is empty does it need an
"ending"? All its senders use it for line/statement ending. If cursor is
at the beginning of a collection, this becomes a null-op. This
interpretation is also consistent with that of Squeak.

> You've modified it to mean that an empty collection doesn't have the
> object appended.

Yes, based on its usage. Otherwise, every new logger starts with a blank
line :-(.

> Is that really what the method is supposed to mean?  The comment matched
> the previous implementation, with your change, it doesn't.
>
> If the change does go ahead, the comment should be updated to reflect
> the changed definition.

Good catch! I will update comments.

> I'm also wondering why you've changed the test to hard-code a string
> stream?

I just took a common usecase from CCodeGenerator.

Regards .. Subbu

On Tue, Apr 18, 2017 at 10:39:34PM +0530, K K Subbu wrote:
> On Tuesday 18 April 2017 09:51 PM, Alistair Grant wrote:
> >This appears to be changing the meaning of #ensureEndsWith:.
> >The previous meaning was that it always ends with the supplied object.
>
> The method name is ambiguous. If the collection is empty does it need an
> "ending"? All its senders use it for line/statement ending. If cursor is at
> the beginning of a collection, this becomes a null-op. This interpretation
> is also consistent with that of Squeak.

Being consistent with Squeak sounds like a good argument.


> >You've modified it to mean that an empty collection doesn't have the
> >object appended.
>
> Yes, based on its usage. Otherwise, every new logger starts with a blank
> line :-(.

Sure, but... :-)

1. This is assuming that ensureEndsWith: is only used for strings, but
the implementation doesn't assume that (I tested it with an Array and it
was fine).
2. If you look at existing senders of ensureEndsWith: they check that
the string isn't empty.

Having said that, I'm not opposed to the change (especially if it
improves interoperability between the various Smalltalks), just that we
think about the wider implications.

Cheers,
Alistair

On Tuesday 18 April 2017 09:17 PM, Blondeau Vincent wrote:
> Hi,
>
> Thanks for reporting!
>
> If you want to contribute and have your fix integrated, I advise you
> to follow the procedure here: http://pharo.org/contribute-propose-fix

Thanks. I opened an issue and posted my fix for review at:

https://pharo.fogbugz.com/f/cases/19949

Change Sorter tool gave me some grief :-(. It didn't accept changes to
preamble. I finally had to do

  (ChangeSet named: 'blahblah.cs') editPreamble

to prepare the changeset. The error exists in Pharo 6 also.

Regards .. Subbu

On Tuesday 18 April 2017 10:51 PM, Alistair Grant wrote:
> 1. This is assuming that ensureEndsWith: is only used for strings, but
> the implementation doesn't assume that (I tested it with an Array and it
> was fine).
> 2. If you look at existing senders of ensureEndsWith: they check that
> the string isn't empty.

In Pharo 5/6 WriteStream>>ensureCr does not. I guess this is one of
those annoyances that most people just overlook. I happened to use
generator.image from the command line in a small terminal window and got
annoyed with all those blank lines. Chasing it down was more of a fun
exercise in using Pharo.

I really appreciate you reviewing the code and your comments really made
me study the fix more carefully. Thanks.

Regards .. Subbu