Mantis Bugtracker
  

Viewing Issue Simple Details Jump to Notes ] View Advanced ] Issue History ] Print ]
ID Category Severity Reproducibility Date Submitted Last Update
0001722 [Squeak] Collections minor always 08-20-05 02:39 12-18-08 08:00
Reporter mzimmerm View Status public  
Assigned To
Priority normal Resolution open  
Status feedback   Product Version
Summary 0001722: [ENH] Associations Please review and read the tests
Description 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
Additional Information
Attached Files  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

- Relationships
related to 0001711feedback  [FIX] Re: Refactoring Associations 

- 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 *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.
 

- 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


Mantis 1.0.8[^]
Copyright © 2000 - 2007 Mantis Group
111 total queries executed.
59 unique queries executed.
Powered by Mantis Bugtracker