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
-----Message d'origine----- De : Pharo-dev [mailto:pharo-dev-bounces@lists.pharo.org] De la part de K K Subbu Envoyé : mardi 18 avril 2017 17:34 à : Pharo Development List Objet : [Pharo-dev] 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
!!!************************************************************************************* "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.!!!"
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
Hi Subbu, On Tue, Apr 18, 2017 at 09:04:00PM +0530, K K Subbu wrote:
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
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
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 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
participants (3)
-
Alistair Grant -
Blondeau Vincent -
K K Subbu