Mantis - Squeak
Viewing Issue Advanced Details
1722 Collections minor always 08-20-05 02:39 12-18-08 08:00
mzimmerm  
 
normal  
feedback  
open  
none    
none  
0001722: [ENH] Associations Please review and read the tests
Stephane Ducasse <ducasse@iam.unibe.ch>:

"hi

andrew spent a lot time producing a cs that does not break to refactor
Association classes. I reviewed it and I would like that other
compiler-oriented people takes 5 min to review this code (else I'm sure
that someone will complain in 3 months). So please do it now. I
reviewed and loaded everything in 3.8g-6548 and it works.
"




Stef
related to 0001711feedback  [FIX] Re: Refactoring Associations 
 AssociationRefact.Hand.6.cs [^] (10,901 bytes) 08-20-05 02:39
 AsssociationRefactCleanUp.cs [^] (2,110 bytes) 08-20-05 02:40
 AssociationRefactTests.cs [^] (9,640 bytes) 08-20-05 02:41

Notes
(0002439)
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)
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)
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)
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)
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)
mzimmerm   
08-20-05 03:10   
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)
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)
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)
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)
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)
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)
Keith_Hodges   
12-17-08 03:41   
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)
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)
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 *by*design* 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.