[Pharo-project] Replacing 'ReadStream on smth:' by 'smth readStream'
Hi, I've just published in the pharo inbox a package for the System category. This change has been generated automatically through the rewriter: ParseTreeRewriter new replace: 'ReadStream on: ``@object' with: '``@object readStream'; yourself The formatting of the changed method is completely different due to the use of the refactoring browser. This makes the diffs not readable. Even if it is a real problem, I don't see any simple solution. What do you think? -- Damien Cassou Peter von der Ahé: «I'm beginning to see why Gilad wished us good luck». (http://blogs.sun.com/ahe/entry/override_snafu)
The formatting of the changed method is completely different due to the use of the refactoring browser. This makes the diffs not readable. Even if it is a real problem, I don't see any simple solution. What do you think?
Fix the RB. Lol Lukas
-- Damien Cassou Peter von der Ahé: «I'm beginning to see why Gilad wished us good luck». (http://blogs.sun.com/ahe/entry/override_snafu)
_______________________________________________ Pharo-project mailing list Pharo-project@lists.gforge.inria.fr http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-project
-- Lukas Renggli http://www.lukas-renggli.ch
On Fri, May 30, 2008 at 10:57 AM, Lukas Renggli <renggli@gmail.com> wrote:
The formatting of the changed method is completely different due to the use of the refactoring browser. This makes the diffs not readable. Even if it is a real problem, I don't see any simple solution. What do you think?
Fix the RB. Lol
That's why I added 'simple' in the sentence :-). I heard Stéphane said that someone was looking at this. Is there any news? -- Damien Cassou Peter von der Ahé: «I'm beginning to see why Gilad wished us good luck». (http://blogs.sun.com/ahe/entry/override_snafu)
Have you verified that your change does not inadvertently modify any behavior? Its not likely, but changes like this can introduce subtle bugs. For instance, Matrix overrides readStream. So if there exists any use of "ReadStream on: aMatrix" that is there to bypass the overridden behavior, the code will break. Cheers, Adrian On May 30, 2008, at 10:47 , Damien Cassou wrote:
Hi,
I've just published in the pharo inbox a package for the System category. This change has been generated automatically through the rewriter:
ParseTreeRewriter new replace: 'ReadStream on: ``@object' with: '``@object readStream'; yourself
The formatting of the changed method is completely different due to the use of the refactoring browser. This makes the diffs not readable. Even if it is a real problem, I don't see any simple solution. What do you think?
-- Damien Cassou Peter von der Ahé: «I'm beginning to see why Gilad wished us good luck». (http://blogs.sun.com/ahe/entry/override_snafu)
_______________________________________________ Pharo-project mailing list Pharo-project@lists.gforge.inria.fr http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-project
On Fri, May 30, 2008 at 2:18 PM, Adrian Lienhard <adi@netstyle.ch> wrote:
Have you verified that your change does not inadvertently modify any behavior? Its not likely, but changes like this can introduce subtle bugs. For instance, Matrix overrides readStream. So if there exists any use of "ReadStream on: aMatrix" that is there to bypass the overridden behavior, the code will break.
It should not break anything (if it does, the test will fail and I will fix things :-)). In fact, ReadStream only deals with SequenceableStreams so it should be ok. Do you agree? -- Damien Cassou Peter von der Ahé: «I'm beginning to see why Gilad wished us good luck». (http://blogs.sun.com/ahe/entry/override_snafu)
On May 30, 2008, at 15:37 , Damien Cassou wrote:
On Fri, May 30, 2008 at 2:18 PM, Adrian Lienhard <adi@netstyle.ch> wrote:
Have you verified that your change does not inadvertently modify any behavior? Its not likely, but changes like this can introduce subtle bugs. For instance, Matrix overrides readStream. So if there exists any use of "ReadStream on: aMatrix" that is there to bypass the overridden behavior, the code will break.
It should not break anything (if it does, the test will fail and I will fix things :-)). In fact, ReadStream only deals with SequenceableStreams so it should be ok. Do you agree?
No I don't agree. Because it is not likely that there is a test for Matrix that will show a potential bug. I just wanted to point out that it is important to precisely look at the effect of such "global" transformations. (E.g. like Edgar tried to replace all #== with #= IIRC ;) With your proposed change, I think it is unlikely you will break anything, but you cannot say for sure without detailled *manual* checking. The difficulty is that you theoretically needed to verify that there is never passed a Matrix instance to "ReadStream on:". Also to consider with such changes is that you may break external code that you don't know of. To sum up, I'm not against such improvements at all, but they have to be done carefully, i.e., the argument that tests will show any problem does not hold. Cheers, Adrian
-- Damien Cassou Peter von der Ahé: «I'm beginning to see why Gilad wished us good luck». (http://blogs.sun.com/ahe/entry/override_snafu)
_______________________________________________ Pharo-project mailing list Pharo-project@lists.gforge.inria.fr http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-project
On Fri, May 30, 2008 at 6:17 PM, Adrian Lienhard <adi@netstyle.ch> wrote:
No I don't agree. Because it is not likely that there is a test for Matrix that will show a potential bug. I just wanted to point out that it is important to precisely look at the effect of such "global" transformations. (E.g. like Edgar tried to replace all #== with #= IIRC ;)
With your proposed change, I think it is unlikely you will break anything, but you cannot say for sure without detailled *manual* checking. The difficulty is that you theoretically needed to verify that there is never passed a Matrix instance to "ReadStream on:".
I don't understand why you are choosing this example of Matrix. Since Matrix is not an Array nor a String, it is impossible that a code like 'ReadStream on: aMatrix' ever worked. In fact, this reasoning could be applied to any other class which is neither Array nor String or one of their subclasses.
Also to consider with such changes is that you may break external code that you don't know of.
How? If don't remove any method. Existing code should still work. Do you have any example in mind?
To sum up, I'm not against such improvements at all, but they have to be done carefully, i.e., the argument that tests will show any problem does not hold.
I knew the tests argument did not hold because there are so few tests in Squeak, I was joking :-). I'm trying to do the changes carrefully, that's why I commited things package after package instead of doing it globally. -- Damien Cassou Peter von der Ahé: «I'm beginning to see why Gilad wished us good luck». (http://blogs.sun.com/ahe/entry/override_snafu)
I agree with adrian. For pharo we should move but we should also make sure it is really stable. Stef On May 30, 2008, at 2:18 PM, Adrian Lienhard wrote:
Have you verified that your change does not inadvertently modify any behavior? Its not likely, but changes like this can introduce subtle bugs. For instance, Matrix overrides readStream. So if there exists any use of "ReadStream on: aMatrix" that is there to bypass the overridden behavior, the code will break.
Cheers, Adrian
On May 30, 2008, at 10:47 , Damien Cassou wrote:
Hi,
I've just published in the pharo inbox a package for the System category. This change has been generated automatically through the rewriter:
ParseTreeRewriter new replace: 'ReadStream on: ``@object' with: '``@object readStream'; yourself
The formatting of the changed method is completely different due to the use of the refactoring browser. This makes the diffs not readable. Even if it is a real problem, I don't see any simple solution. What do you think?
-- Damien Cassou Peter von der Ahé: «I'm beginning to see why Gilad wished us good luck». (http://blogs.sun.com/ahe/entry/override_snafu)
_______________________________________________ Pharo-project mailing list Pharo-project@lists.gforge.inria.fr http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-project
_______________________________________________ Pharo-project mailing list Pharo-project@lists.gforge.inria.fr http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-project
I agree with adrian. I would like to use the stream as a test for Call For Improvement Process. So could you come up with a kind of template Summary: Key points: Some tests to illustrate Actions/Process Stef On May 30, 2008, at 6:17 PM, Adrian Lienhard wrote:
On May 30, 2008, at 15:37 , Damien Cassou wrote:
On Fri, May 30, 2008 at 2:18 PM, Adrian Lienhard <adi@netstyle.ch> wrote:
Have you verified that your change does not inadvertently modify any behavior? Its not likely, but changes like this can introduce subtle bugs. For instance, Matrix overrides readStream. So if there exists any use of "ReadStream on: aMatrix" that is there to bypass the overridden behavior, the code will break.
It should not break anything (if it does, the test will fail and I will fix things :-)). In fact, ReadStream only deals with SequenceableStreams so it should be ok. Do you agree?
No I don't agree. Because it is not likely that there is a test for Matrix that will show a potential bug. I just wanted to point out that it is important to precisely look at the effect of such "global" transformations. (E.g. like Edgar tried to replace all #== with #= IIRC ;)
With your proposed change, I think it is unlikely you will break anything, but you cannot say for sure without detailled *manual* checking. The difficulty is that you theoretically needed to verify that there is never passed a Matrix instance to "ReadStream on:".
Also to consider with such changes is that you may break external code that you don't know of. To sum up, I'm not against such improvements at all, but they have to be done carefully, i.e., the argument that tests will show any problem does not hold.
Cheers, Adrian
-- Damien Cassou Peter von der Ahé: «I'm beginning to see why Gilad wished us good luck». (http://blogs.sun.com/ahe/entry/override_snafu)
_______________________________________________ Pharo-project mailing list Pharo-project@lists.gforge.inria.fr http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-project
_______________________________________________ Pharo-project mailing list Pharo-project@lists.gforge.inria.fr http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-project
excellent!
I knew the tests argument did not hold because there are so few tests in Squeak, I was joking :-). I'm trying to do the changes carrefully, that's why I commited things package after package instead of doing it globally.
Stef
participants (4)
-
Adrian Lienhard -
Damien Cassou -
Lukas Renggli -
Stéphane Ducasse