In this particular case, I'd simply delete the ifFalse:. The other changes are independent of the change by djm; they could have been made even if version 2) never existed. The argument for this can be strengthened because in this case it is clear why every change has been made: 3) to accomodate addition of AbstractString to the class hierarchy 4) because #isString looks better and/or is faster 5) because underscores are no longer allowed for assignment) ... and none of these motivations is in any way related to the change by djm.
Then, I'd advertise for volunteers to re-write the method so that it works when the receiver contains arbitrary objects, not only when it contains Strings (SequenceableCollections?). A TestCase would help ensure that they get it right.
If this isn't defensible, then I don't know where we would draw the line.
Of course, there are probably many examples that are more "grey" than this one.
Anyway, thanks for highlighting these issues to the community. Cheers! Josh
Ken Causey wrote:
This is going to be a rather long email but I ask you to at least skim if you have even a little interest in the relicensing/Squeak 4.0 effort.
Thanks to the efforts of many including Yoshiki Ohshima and Matthew Fulmer we have made some real progress on a 'license clean' version of Squeak. But we are now at the phase where we have some code for which it appears we are not going to get a relicensing agreement. So at this point it's either remove it or replace it.
Thanks to Matthew publishing the history tools I've started to try to help out with this and have quickly learned that I really don't know what to do. I'm going to present an example here and I'm asking you for your considered opinion.
SequenceableCollection>>asStringWithCr
This method goes back to Squeak1.0 and has 5 versions leading up to what is today in Squeak3.10.2-7197/Squeak4.0alpha. It is the second version by Douglas McPherson that has the problem (we have not received a relicensing agreement from Douglas). I'm going to paste in, oldest to newest, all versions so we can discuss this concretely. In each I will number the version in front of the containing filename for discussion convenience and mark changed lines (relative to the earlier version) with a + in the first column. The filenames used are those as distributed with the Matthew/Yoshiki history tools; sometimes these don't directly map to the original updates (think updates that load MCZs).
- SqueakV1.sources:
asStringWithCr "Convert to a string with returns between items. Elements are usually strings. Useful for labels for PopUpMenus." | labelStream | labelStream _ WriteStream on: (String new: 200). self do: [:each | (each isKindOf: String) ifTrue: [labelStream nextPutAll: each; cr] ifFalse: [each printOn: labelStream; cr]]. self size > 0 ifTrue: [labelStream skip: -1]. ^ labelStream contents!
- 442TweaksForBeta-di.cs:
!SequenceableCollection methodsFor: 'converting' stamp: 'djm 11/20/1998 05:44'! asStringWithCr "Convert to a string with returns between items. Elements are usually strings. Useful for labels for PopUpMenus." | labelStream | labelStream _ WriteStream on: (String new: 200). self do: [:each | (each isKindOf: String) ifTrue: [labelStream nextPutAll: each; cr]
ifFalse: [each printOn: labelStream. labelStream cr]].
- 5994-004-systemMod.cs:
!SequenceableCollection methodsFor: 'converting' stamp: 'yo 8/28/2002 15:39'! asStringWithCr "Convert to a string with returns between items. Elements are usually strings. Useful for labels for PopUpMenus." | labelStream | labelStream _ WriteStream on: (String new: 200). self do: [:each |
(each isKindOf: AbstractString) ifTrue: [labelStream nextPutAll: each; cr] ifFalse: [each printOn: labelStream. labelStream cr]].
- 6651ExternalCleanup.cs:
!SequenceableCollection methodsFor: 'converting' stamp: 'ar 4/10/2005 18:02'! asStringWithCr "Convert to a string with returns between items. Elements are usually strings. Useful for labels for PopUpMenus." | labelStream | labelStream _ WriteStream on: (String new: 200). self do: [:each |
each isString ifTrue: [labelStream nextPutAll: each; cr] ifFalse: [each printOn: labelStream. labelStream cr]].
- Squeak39g-7056+3102-7179.changes:
!SequenceableCollection methodsFor: 'converting' stamp: 'ar 4/10/2005 18:02' prior: 29748463! asStringWithCr "Convert to a string with returns between items. Elements are usually strings. Useful for labels for PopUpMenus." | labelStream |
- labelStream := WriteStream on: (String new: 200). self do: [:each | each isString ifTrue: [labelStream nextPutAll: each; cr] ifFalse: [each printOn: labelStream. labelStream cr]]. self size > 0 ifTrue: [labelStream skip: -1]. ^ labelStream contents! !
Now as I said above the problem with with version 2. As you can see each of these changes is at most one line and by and large frankly rather obvious. The change in 2 is clearly a bug fix and the straightforward solution.
So the question is what do I do with this method, which is currently distributed as version 5, so that it does not infringe on djm's rights to maintain version 2 as SqueakL? Admittedly it's unlikely that djm actually wants this right, but lacking a clear statement to the contrary, it's the default we have to assume.
How much of a change is a significant change?
Should I delete the current implementation entirely and rewrite it from scratch? If so, how much am I allowed to know about the prior implementations?
Ken