0001722: [ENH] Associations Please review and read the tests

Notes
(0002439 - 671 - 835 - 883 - 883 - 883 - 883) mzimmerm 08-20-05 02:41	Brent Pinkney <brent.pinkney@aircom.co.za>: "Hi, I had a look at this code since installing the Chronology classes required similar gymnastics not to break the image. 1. It would be nice to know soem background which motivated these changes. 2. The selector AssociationTests>>testtReadOnlyAssocEquality is misspelt. 3. Some of the tests use abbreviated names, some don't. For readability for Those Who May Follow Us, raname methods like testReadOnlyAssoc to testReadOnlyAssociation. 4. Could these three .cs files not be bundled into one changeset ? 5. Do we really need ReadOnlyVariableBinding ? I tested this in 3.9alpha-6548. All ok. "

(0002440 - 1061 - 1302 - 1344 - 1344 - 1344 - 1344) mzimmerm 08-20-05 02:44	Stephane Ducasse <ducasse@iam.unibe.ch>: " On 14 févr. 05, at 6:17, Brent Pinkney wrote: > Hi, > > I had a look at this code since installing the Chronology classes > required similar gymnastics not to break the image. > > 1. It would be nice to know soem background which motivated these > changes. > 2. The selector AssociationTests>>testtReadOnlyAssocEquality is > misspelt. > 3. Some of the tests use abbreviated names, some don't. > For readability for Those Who May Follow Us, raname methods like > testReadOnlyAssoc to > testReadOnlyAssociation. > 4. Could these three .cs files not be bundled into one changeset ? > 5. Do we really need ReadOnlyVariableBinding ? I think that andrew should let us know why this is better (I imagine that this is better than a class implementeing that in your back, at least when I read its name I know what it is about) and I checked and all the class interfaces are the same, so normally the changes should be compatible with the rest of the system and do not break. Stef "

(0002441 - 2557 - 3113 - 3261 - 3261 - 3261 - 3261) mzimmerm 08-20-05 02:48	Stephane Ducasse <ducasse@iam.unibe.ch>: (this is forwarded) " > Let me give you my side of it. > > The reason I started mucking with the association code is that it is > BROKEN. How? Run the tests on the existing image and you will see. > There are many missing methods in the various "special" association > classes, because they do not inherit from Association, but from > LookUpKey. > > I didn't invent ReadOnlyVariable binding. But it is there, and it us > used. Unfortunately, because the inheritance hierarchy is presently > wrong, ReadOnlyvariableBindings appear in the debugger like > LookUpKeys, not like associations. I lost many hours that way. > > So I fixed it. But I don't like to patch, I like to do it right. So > I wrote tests, made the existing code fail, and then wrote new > classes, which made the tests pass. I thought that this was the > "good" way to do it. > > Then I had to make a changeset so that this stuff would get out into > the image. This was a big job, because of all of the special case > code that was trying to stop someone from fixing these bugs. Building > the change set took weeks. The system code is simple and robust, but > the change set is not. It was built with bit tweezers (a text editor) > because Squeak has no facilities for ordering changes or committing > changes atomically. > > Then I hoped that it would get into the image. I have had > conversations with several different harvesters about the whys and the > wherefores of these changes. But they never get into the image. We > are now two versions away from the Squeak where I made this changeset > work, and so I'm nor surprised that it doesn't work for you. > > My suggestion from this are: > > (1) If we need special case code to stop things from being recompiled, > there should be a way to turn it off. > > (2) I was told that the right way to document the need for changes is > to create a failing test set and submit it. I did this in this case, > but it has apparently been lost. So, we should decide what IS the > right way to document the need for a change. > > (3) There should be an accelerated path for getting "delicate" change > sets like this into the image. > > Andrew > > -- > > Prof. Andrew P. Black Home: +1 503 629 5495 > Department of Computer Science, Cell: +1 503 803 1669 > Portland State University Office: +1 503 725 2411 > Portland, Oregon, USA http://www.cs.pdx.edu/~black [^] > "

(0002442 - 396 - 488 - 488 - 488 - 488 - 488) mzimmerm 08-20-05 02:55	frank.shearar@rnid.org.uk: " ( [er] Looks good to my inexperienced eye ) Unlike Brent, I'm OK with using "Assoc" as an abbreviation - the expansion seems obvious to me (a native English speaker). What do Germans, Japanese, etc., Squeakers think? All the tests ran green in 3.8g-6550. Eyeball-wise, the code looks fine. Some comments have spelling mistakes, but I'll live :) frank"

(0002443 - 456 - 601 - 797 - 797 - 797 - 797) mzimmerm 08-20-05 03:00	Markus Fritsche <Fritsche.Markus@gmx.net>: "frank.shearar@rnid.org.uk wrote: > Unlike Brent, I'm OK with using "Assoc" as an abbreviation - the > expansion seems obvious to me (a native English speaker). What do > Germans, Japanese, etc., Squeakers think? If I fire up the image, the common use till now is to use "association", theres only one implementor of "assoc:", GlobalVar. Best regards, Markus -- http://reauktion.de/archer/ [^] "

(0002444 - 1102 - 1486 - 1625 - 1625 - 1625 - 1625) mzimmerm 08-20-05 03:10 edited on: 08-20-05 03:13	Frank Shearar <Frank.Shearar@rnid.org.uk>: " Markus Fritsche <Fritsche.Markus@gmx.net> wrote: > > frank.shearar@rnid.org.uk wrote: > > > Unlike Brent, I'm OK with using "Assoc" as an abbreviation - the > > expansion seems obvious to me (a native English speaker). What do > > Germans, Japanese, etc., Squeakers think? > > If I fire up the image, the common use till now is to use > "association", > theres only one implementor of "assoc:", GlobalVar. Brent was referring to a number of test methods with names like testReadOnlyAssocPrinting and the like. I don't mind a name like that because you're in the AssociationTests - you KNOW you're reading about Associations, so seeing "Assoc" in a method name shouldn't confuse you. On closer inspection though, there is something wrong with the test codes. As an example: testReadOnlyAssocPrinting "self run: #testAssociationPrinting" \| a c s \| <blah blah> The comment doesn't match the method name. Affected tests are: #testReadOnlyAssocPrinting #testWeakKeyAssocPrinting #testWeakValueAssocPrinting frank "

(0002445 - 538 - 741 - 836 - 836 - 836 - 836) mzimmerm 08-20-05 03:14	Brent Pinkney <brent.pinkney@aircom.co.za>: " <Frank.Shearar@rnid.org.uk> wrote: > > testReadOnlyAssocPrinting > "self run: #testAssociationPrinting" > \| a c s \| > <blah blah> > > The comment doesn't match the method name. Affected tests are: > > #testReadOnlyAssocPrinting > #testWeakKeyAssocPrinting > #testWeakValueAssocPrinting That is what I meant :). I would prefer to fix it by changing the meothd selectors to use Association and not change the comment. Written once but are read many times. Brent "

(0002446 - 673 - 755 - 755 - 755 - 755 - 755) mzimmerm 08-20-05 03:17	rastaehli@mac.com: "Despite the obscurity of some of the LookupKey subclasses, this is a straightforward refactoring to improve documentation and fix obvious (see Andrew's test suite) bugs in the old classes. The most worrisome aspect of the change set is the delicacy of converting all instances of the old classes to the new. The comments in the changeset make the strategy clear enough, though I could not follow all the details. Testing on a clean Squeak3.8g-6548.image shows 1334/8/3 passed/failed/errors before the attached changes sets are loaded and 1349/8/3 after, indicating that all Andrew's new tests pass and that none of the old tests break. "

(0002447 - 451 - 538 - 538 - 538 - 538 - 538) mzimmerm 08-20-05 03:20	rastaehli@mac.com: " ( [er][su] passes new tests, does not break old in clean Squeak3.8g-6548.image ) This is a good refactoring and bug fix to LookupKey subclasses. In clean Squeak3.8g-6548.image before loading the attached change sets, the number of passed/fail/error tests is 1334/8/3 and after the change sets the number is 1349/8/3. apologies if this is a duplicate submission, but I've been having trouble with first BVAV use. "

(0002450 - 278 - 365 - 365 - 365 - 365 - 365) mzimmerm 08-20-05 03:36	When moving this item from BFAV into Mantis, I verified the attached changesets install into 3.8-6665. It appears they need to be installed in this order (otherwise an error appears): - AssociationRefact.Hand.6.cs - AssociationRafactCleanUp.cs - AssociationRefactTests.cs.

(0002451 - 451 - 541 - 541 - 541 - 541 - 541) mzimmerm 08-20-05 03:57	This item (1722) appears to be already in Mantis under ID 1711. However, the 1711 - has a different set of changesets (4 changesets on 1711, 3 changesets on this ID) - timestamp on 1711 is much earlier in BFAV (year 2004 on 1711 as opposed to year 2005 on this ID) As a result, I am not sure which of those is "correct", I am asking Ken who has permissions to "link them" here on Mantis. Then someone with more experience can make a judgement.

(0012856 - 216 - 228 - 228 - 228 - 228 - 228) Keith_Hodges 12-17-08 03:41 edited on: 12-17-08 03:58	3.11 Feedback - Need to review the code relative to 3.10.2bc. We have atomic loading for change sets. Name the change set .mcs so that monticello will load it, and load with Installer (with atomic loading enabled).

(0012863 - 229 - 229 - 229 - 229 - 229 - 229) mzimmerm 12-18-08 02:44	How do I load the changesets with Installer? I am also unsure how to test the result of installing these (I'm not the original author, I merely moved this from BFAV to Mantis). I will take a look when I get back in about 10 days.

(0012868 - 2397 - 2613 - 2613 - 2613 - 2613 - 2613) kwl 12-18-08 08:00	O.K. you asked for review and here is mine: - documentation no word is added to the class comment of good old Association and there people are not made aware of what to use for what. - #convertToAPBClass the implementation of this method and its call site are unacceptable (at best). this is not Java-land here, a Smalltalker can and must do better. I have nothing against people with "for me it works" mentality but am strictly against causing problems in larger and especially production size .images out there in the wild. - AbstractAssociation>>#key:value: being an abstract class this method cannot know that #key:value: has to resolve to (self key: ...) and (self value: ...), same with #privateSetKey:. another issue is that (super key: ...) is not sent and so superclass LookupKey's behavior is effectively bypassed (encapsulation, anybody?). this also documents that AbstractAssociation is the wrong superclass bydesign* for the things below it. recommendation (in addition to no longer disregarding LookupKey's responsibility): a) implement #key:value: in subclasses of AbstractAssociation b) remove #key:value: from AbstractAssociation c) alternately (to b) mark #key:value: in AbstractAssociation as #subclassResponsibility - who will understand this in 3 years from now one of the method returns the value for the #key message and another assigns to key for the #value method. for me this killed the whole approach. these methods have to be renamed for reflecting the difference and the intention. - unwanted side effects any change to LookupKey, the superclass of AbstractAssociation, is unaccaptable, other than for enhancing performance or functionality, which is not the case with these changes. also, the critical changes to the environment are not undone, so that every newbee can now begin to hack critical system classes. - critique another reviewer compared # of test/status before and after, but not in the sense of taking 3 good apples out of the basket and putting 3 bad ones into the basket. also, reviewers mentioned "unclear" together with "strategy". all such doubts have, so I think, to be removed. - conclusion these changesets are not for comsumption by the masses, in particular not for automated inclusion by Installer and friends. but they are blueprint for making incremental .mcz changes in the spirit of Smalltalk.

Issue History
Date Modified	Username	Field	Change
08-20-05 02:39	mzimmerm	New Issue
08-20-05 02:39	mzimmerm	File Added: AssociationRefact.Hand.6.cs
08-20-05 02:40	mzimmerm	File Added: AsssociationRefactCleanUp.cs
08-20-05 02:41	mzimmerm	File Added: AssociationRefactTests.cs
08-20-05 02:41	mzimmerm	Note Added: 0002439
08-20-05 02:44	mzimmerm	Note Added: 0002440
08-20-05 02:48	mzimmerm	Note Added: 0002441
08-20-05 02:55	mzimmerm	Note Added: 0002442
08-20-05 03:00	mzimmerm	Note Added: 0002443
08-20-05 03:10	mzimmerm	Note Added: 0002444
08-20-05 03:13	mzimmerm	Note Edited: 0002444
08-20-05 03:14	mzimmerm	Note Added: 0002445
08-20-05 03:17	mzimmerm	Note Added: 0002446
08-20-05 03:20	mzimmerm	Note Added: 0002447
08-20-05 03:36	mzimmerm	Note Added: 0002450
08-20-05 03:57	mzimmerm	Note Added: 0002451
08-22-05 18:29	KenCausey	Relationship added	related to 0001711
12-17-08 03:41	Keith_Hodges	Note Added: 0012856
12-17-08 03:55	Keith_Hodges	Note Edited: 0012856
12-17-08 03:58	Keith_Hodges	Note Edited: 0012856
12-17-08 03:58	Keith_Hodges	Note Edited: 0012856
12-17-08 03:59	Keith_Hodges	Status	new => feedback
12-18-08 02:44	mzimmerm	Note Added: 0012863
12-18-08 08:00	kwl	Note Added: 0012868