The Trunk: Collections-dtl.276.mcz

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
8 messages Options
Reply | Threaded
Open this post in threaded view
|

The Trunk: Collections-dtl.276.mcz

commits-2
David T. Lewis uploaded a new version of Collections to project The Trunk:
http://source.squeak.org/trunk/Collections-dtl.276.mcz

==================== Summary ====================

Name: Collections-dtl.276
Author: dtl
Time: 9 January 2010, 6:12:34 am
UUID: 77fae7af-c5f8-4fe8-b328-680cdd71115f
Ancestors: Collections-ar.275

Preserve #fullCheck for compatibility with external packages (i.e., extension methods in Dictionary etc). Important for SystemTracing package loading.

=============== Diff against Collections-ar.275 ===============

Item was added:
+ ----- Method: HashedCollection>>fullCheck (in category 'compatibility') -----
+ fullCheck
+ "This is a private method, formerly implemented in Set, that is no longer required.
+ It is here for compatibility with external packages only."
+ ^ self
+ !


Reply | Threaded
Open this post in threaded view
|

Re: The Trunk: Collections-dtl.276.mcz

Levente Uzonyi-2


On Sat, 9 Jan 2010, [hidden email] wrote:

> David T. Lewis uploaded a new version of Collections to project The Trunk:
> http://source.squeak.org/trunk/Collections-dtl.276.mcz
>
> ==================== Summary ====================
>
> Name: Collections-dtl.276
> Author: dtl
> Time: 9 January 2010, 6:12:34 am
> UUID: 77fae7af-c5f8-4fe8-b328-680cdd71115f
> Ancestors: Collections-ar.275
>
> Preserve #fullCheck for compatibility with external packages (i.e., extension methods in Dictionary etc). Important for SystemTracing package loading.
>

Why aren't these methods in the 311Deprecated package?


Levente

> =============== Diff against Collections-ar.275 ===============
>
> Item was added:
> + ----- Method: HashedCollection>>fullCheck (in category 'compatibility') -----
> + fullCheck
> + "This is a private method, formerly implemented in Set, that is no longer required.
> + It is here for compatibility with external packages only."
> + ^ self
> + !
>
>
>

Reply | Threaded
Open this post in threaded view
|

Re: The Trunk: Collections-dtl.276.mcz

David T. Lewis
On Sun, Jan 10, 2010 at 03:09:10AM +0100, Levente Uzonyi wrote:

>
> On Sat, 9 Jan 2010, [hidden email] wrote:
>
> >David T. Lewis uploaded a new version of Collections to project The Trunk:
> >http://source.squeak.org/trunk/Collections-dtl.276.mcz
> >
> >==================== Summary ====================
> >
> >Name: Collections-dtl.276
> >Author: dtl
> >Time: 9 January 2010, 6:12:34 am
> >UUID: 77fae7af-c5f8-4fe8-b328-680cdd71115f
> >Ancestors: Collections-ar.275
> >
> >Preserve #fullCheck for compatibility with external packages (i.e.,
> >extension methods in Dictionary etc). Important for SystemTracing package
> >loading.
> >
>
> Why aren't these methods in the 311Deprecated package?

Good question, and I'm not sure which is better in this case. I think
the choice should be driven by whether we view these as deprecated,
implying that they would be removed perhaps a year or so from now, or
if they are intended as compatability methods that might stay in the
image for a longer period of time.

>From the point of view of an external package maintainer, it is
important for a package to be loadable in a range of images. As a
rule of thumb, I try to provide backward compatibility at least back
to Squeak 3.8. That's a time window of about five years, so I expect
methods intended for compatabily to be available for at least that
long, and I expect deprecated methods to go away after about a year.

As a platform maintainer, my personal pet peeve is the stuff that
got moved into SmalltalkImage, which forced me to write horrible
workarounds in several packages to deal with the "improvement".
For example in OSProcess I have things like this:

  platformName
    "After Squeak version 3.6, #platformName was moved to SmalltalkImage "
     ^ ((Smalltalk classNamed: 'SmalltalkImage')
         ifNil: [^ Smalltalk platformName]) current platformName

This might seem like a minor complaint, but it is actually very
time consuming to test an external package on a range of images.
I for one do not appreciate being asked to keep up with changes
that provide no functional benefit and waste a lot of my time.

I'm pretty sure that the folks doing e.g. Seaside are going to feel
the same way, so we need to be careful to provide compatibility
methods where appropriate, but not go overboard with it and clutter
up the image with junk. So, to return to the original question,
I'm not sure if these methods should be moved to 311Deprecated,
but if the intent is to keep them for say five years for compatibility,
then no they should not be moved.

Dave
 

Reply | Threaded
Open this post in threaded view
|

Re: The Trunk: Collections-dtl.276.mcz

Andreas.Raab
David T. Lewis wrote:
> On Sun, Jan 10, 2010 at 03:09:10AM +0100, Levente Uzonyi wrote:
>> Why aren't these methods in the 311Deprecated package?
>
> Good question, and I'm not sure which is better in this case. I think
> the choice should be driven by whether we view these as deprecated,
> implying that they would be removed perhaps a year or so from now, or
> if they are intended as compatability methods that might stay in the
> image for a longer period of time.

I agree. The reason why I put findElementOrNil: back right away was that
it's being used by DynamicBindings of which a multitude of versions
exists in many places on the web. It's virtually impossible to fix all
the existing versions.

> I'm pretty sure that the folks doing e.g. Seaside are going to feel
> the same way, so we need to be careful to provide compatibility
> methods where appropriate, but not go overboard with it and clutter
> up the image with junk. So, to return to the original question,
> I'm not sure if these methods should be moved to 311Deprecated,
> but if the intent is to keep them for say five years for compatibility,
> then no they should not be moved.

+1 on all of it. We should get rid of these methods whenever we can but
we also need to take into account the existence of external packages. In
the case of findElementOrNil: I first found (and fixed) a usage in
Tweak, then I found (and fixed) a usage in DynamicBindings on
http://source.squeak.org/ss but then I ran into one usage via
DynamicBindings in a package loaded three indirections deep via
Metacello. That's what pushed it over the edge for me - clearly there's
more than one version out there and it's hard to predict which one
people will attempt to load.

Cheers,
   - Andreas

Reply | Threaded
Open this post in threaded view
|

Compatibility with external packages (was: Re: The Trunk: Collections-dtl.276.mcz)

Levente Uzonyi-2
In reply to this post by David T. Lewis
On Sun, 10 Jan 2010, David T. Lewis wrote:

> On Sun, Jan 10, 2010 at 03:09:10AM +0100, Levente Uzonyi wrote:
>>
>> On Sat, 9 Jan 2010, [hidden email] wrote:
>>
>>> David T. Lewis uploaded a new version of Collections to project The Trunk:
>>> http://source.squeak.org/trunk/Collections-dtl.276.mcz
>>>
>>> ==================== Summary ====================
>>>
>>> Name: Collections-dtl.276
>>> Author: dtl
>>> Time: 9 January 2010, 6:12:34 am
>>> UUID: 77fae7af-c5f8-4fe8-b328-680cdd71115f
>>> Ancestors: Collections-ar.275
>>>
>>> Preserve #fullCheck for compatibility with external packages (i.e.,
>>> extension methods in Dictionary etc). Important for SystemTracing package
>>> loading.
>>>
>>
>> Why aren't these methods in the 311Deprecated package?
>
> Good question, and I'm not sure which is better in this case. I think
> the choice should be driven by whether we view these as deprecated,
> implying that they would be removed perhaps a year or so from now, or
> if they are intended as compatability methods that might stay in the
> image for a longer period of time.

There's no guarantee that old code works even if all previously removed
methods are added again. For example if a subclass of Set in an
external package has it's own version of #findElementOrNil: or #fullCheck,
it may have no effect, since it will have no senders in Set. I think that
the only way is to update external packages and use them. So far the only
package I fixed is DynamicBindings, the fixed version is available here:
http://squeaksource.com/KomHttpServer/DynamicBindings-ul.12.mcz .

>
>> From the point of view of an external package maintainer, it is
> important for a package to be loadable in a range of images. As a
> rule of thumb, I try to provide backward compatibility at least back
> to Squeak 3.8. That's a time window of about five years, so I expect
> methods intended for compatabily to be available for at least that
> long, and I expect deprecated methods to go away after about a year.

Yes, packages should be loadable, but sometimes this requires the
migration of packages. I don't think that the latest version of a
package should support all image versions.

>
> As a platform maintainer, my personal pet peeve is the stuff that
> got moved into SmalltalkImage, which forced me to write horrible
> workarounds in several packages to deal with the "improvement".
> For example in OSProcess I have things like this:
>
>  platformName
>    "After Squeak version 3.6, #platformName was moved to SmalltalkImage "
>     ^ ((Smalltalk classNamed: 'SmalltalkImage')
>         ifNil: [^ Smalltalk platformName]) current platformName
>

What about this?
SystemDictionary >> orSmalltalkImage

  ^(self at: #SmalltalkImage ifAbsent: [ ^self ]) current

Which would let us write: Smalltalk orSmalltalkImage platformName.

> This might seem like a minor complaint, but it is actually very
> time consuming to test an external package on a range of images.
> I for one do not appreciate being asked to keep up with changes
> that provide no functional benefit and waste a lot of my time.
>
> I'm pretty sure that the folks doing e.g. Seaside are going to feel
> the same way, so we need to be careful to provide compatibility
> methods where appropriate, but not go overboard with it and clutter
> up the image with junk. So, to return to the original question,
> I'm not sure if these methods should be moved to 311Deprecated,
> but if the intent is to keep them for say five years for compatibility,
> then no they should not be moved.

As I noted above, having these methods in the image do not ensure 100%
backwards compatibility with external packages.
Five years may be too much, noone will remember in 2015 that some
methods/classes can be removed from the image.
Sometimes it's easier to fix the external packages (like
the DynamicBindings case, where the fix is fully backwards compatible).
I think we should produce a migration guide, like the Seaside developers:
http://www.seaside.st/documentation/migration . This could also include a
list of deprecated features (methods/classes).


Levente

>
> Dave
>
>
>

Reply | Threaded
Open this post in threaded view
|

Re: Re: The Trunk: Collections-dtl.276.mcz

Levente Uzonyi-2
In reply to this post by Andreas.Raab
On Sun, 10 Jan 2010, Andreas Raab wrote:

> David T. Lewis wrote:
>> On Sun, Jan 10, 2010 at 03:09:10AM +0100, Levente Uzonyi wrote:
>>> Why aren't these methods in the 311Deprecated package?
>>
>> Good question, and I'm not sure which is better in this case. I think
>> the choice should be driven by whether we view these as deprecated,
>> implying that they would be removed perhaps a year or so from now, or
>> if they are intended as compatability methods that might stay in the
>> image for a longer period of time.
>
> I agree. The reason why I put findElementOrNil: back right away was that it's
> being used by DynamicBindings of which a multitude of versions exists in many
> places on the web. It's virtually impossible to fix all the existing
> versions.
>
>> I'm pretty sure that the folks doing e.g. Seaside are going to feel
>> the same way, so we need to be careful to provide compatibility
>> methods where appropriate, but not go overboard with it and clutter
>> up the image with junk. So, to return to the original question,
>> I'm not sure if these methods should be moved to 311Deprecated,
>> but if the intent is to keep them for say five years for compatibility,
>> then no they should not be moved.
>
> +1 on all of it. We should get rid of these methods whenever we can but we
> also need to take into account the existence of external packages. In the
> case of findElementOrNil: I first found (and fixed) a usage in Tweak, then I
> found (and fixed) a usage in DynamicBindings on http://source.squeak.org/ss 
> but then I ran into one usage via DynamicBindings in a package loaded three
> indirections deep via Metacello. That's what pushed it over the edge for me -
> clearly there's more than one version out there and it's hard to predict
> which one people will attempt to load.

There's a DynamicBindings repositiory at squeaksource.com
(http://squeaksource.com/DynamicBindings.html ) which seems to be
abandoned. AFAIK
the package is maintained at http://squeaksource.com/KomHttpServer.html .


Levente

>
> Cheers,
>  - Andreas
>
>

Reply | Threaded
Open this post in threaded view
|

Re: Compatibility with external packages

Andreas.Raab
In reply to this post by Levente Uzonyi-2
Levente Uzonyi wrote:
> There's no guarantee that old code works even if all previously removed
> methods are added again. For example if a subclass of Set in an external
> package has it's own version of #findElementOrNil: or #fullCheck, it may
> have no effect, since it will have no senders in Set. I think that the
> only way is to update external packages and use them. So far the only
> package I fixed is DynamicBindings, the fixed version is available here:
> http://squeaksource.com/KomHttpServer/DynamicBindings-ul.12.mcz .

I think we're all in violent agreement here. The introduction of such
changes has to be decided in the concrete context. From my point of
view, due to the importance of Seaside we have no choice but to provide
backwards compatibility for DynamicBindings - the failure mode is too
obscure to risk having people run into it.

>>  platformName
>>    "After Squeak version 3.6, #platformName was moved to SmalltalkImage "
>>     ^ ((Smalltalk classNamed: 'SmalltalkImage')
>>         ifNil: [^ Smalltalk platformName]) current platformName
>>
>
> What about this?
> SystemDictionary >> orSmalltalkImage
>
>     ^(self at: #SmalltalkImage ifAbsent: [ ^self ]) current
>
> Which would let us write: Smalltalk orSmalltalkImage platformName.

Eeek! :-) Juan has the right solution for this problem in Cuis:

SmalltalkImage class>>current
        ^Smalltalk

and then leave the methods in Smalltalk which is the only sensible place
anyway. (I'd really like to move things back at some point)

> As I noted above, having these methods in the image do not ensure 100%
> backwards compatibility with external packages.
> Five years may be too much, noone will remember in 2015 that some
> methods/classes can be removed from the image.
> Sometimes it's easier to fix the external packages (like the
> DynamicBindings case, where the fix is fully backwards compatible).
> I think we should produce a migration guide, like the Seaside developers:
> http://www.seaside.st/documentation/migration . This could also include
> a list of deprecated features (methods/classes).

I agree with all of these comments as well (and I think David does,
too). The main issue is: Sometimes we need backwards compatibility.
DynamicBindings is a situation where I strongly feel it's worth it
because it's so widely used in all sorts of old and new Seaside versions.

Cheers,
   - Andreas

Reply | Threaded
Open this post in threaded view
|

Re: Re: Compatibility with external packages

Levente Uzonyi-2
On Sun, 10 Jan 2010, Andreas Raab wrote:

> Levente Uzonyi wrote:
>> There's no guarantee that old code works even if all previously removed
>> methods are added again. For example if a subclass of Set in an external
>> package has it's own version of #findElementOrNil: or #fullCheck, it may
>> have no effect, since it will have no senders in Set. I think that the only
>> way is to update external packages and use them. So far the only package I
>> fixed is DynamicBindings, the fixed version is available here:
>> http://squeaksource.com/KomHttpServer/DynamicBindings-ul.12.mcz .
>
> I think we're all in violent agreement here. The introduction of such changes
> has to be decided in the concrete context. From my point of view, due to the
> importance of Seaside we have no choice but to provide backwards
> compatibility for DynamicBindings - the failure mode is too obscure to risk
> having people run into it.
>

If you're moving your code to a newer image, you can update
DynamicBindings and everything is fine. I agree that it's easier to not
update, but we should motivate people to update. A deprecation warning
would be nice IMO.

>>>  platformName
>>>    "After Squeak version 3.6, #platformName was moved to SmalltalkImage "
>>>     ^ ((Smalltalk classNamed: 'SmalltalkImage')
>>>         ifNil: [^ Smalltalk platformName]) current platformName
>>>
>>
>> What about this?
>> SystemDictionary >> orSmalltalkImage
>>
>>     ^(self at: #SmalltalkImage ifAbsent: [ ^self ]) current
>>
>> Which would let us write: Smalltalk orSmalltalkImage platformName.
>
> Eeek! :-) Juan has the right solution for this problem in Cuis:
>
> SmalltalkImage class>>current
> ^Smalltalk
>
> and then leave the methods in Smalltalk which is the only sensible place
> anyway. (I'd really like to move things back at some point)

That looks nice, but how do you ensure that SmalltalkImage exists?

>
>> As I noted above, having these methods in the image do not ensure 100%
>> backwards compatibility with external packages.
>> Five years may be too much, noone will remember in 2015 that some
>> methods/classes can be removed from the image.
>> Sometimes it's easier to fix the external packages (like the
>> DynamicBindings case, where the fix is fully backwards compatible).
>> I think we should produce a migration guide, like the Seaside developers:
>> http://www.seaside.st/documentation/migration . This could also include a
>> list of deprecated features (methods/classes).
>
> I agree with all of these comments as well (and I think David does, too). The
> main issue is: Sometimes we need backwards compatibility. DynamicBindings is
> a situation where I strongly feel it's worth it because it's so widely used
> in all sorts of old and new Seaside versions.

It's worth to do it, but we should also motivate people to update.

For DynamicBindings, the best would be to take over the DynamicBindings
repository at squeaksource.com and move the developement there from the
KomHttpRepository.


Levente

>
> Cheers,
>  - Andreas
>
>