[Pharo-project] AddMethodRefactoring>>preconditions
Lukas, i stumbled upon a strange behavior using the refactoring engine. When adding a method, the precondition checks if the class or any super class defines the method to be added. It must only ask if the class directly defines it. The fix is AddMethodRefactoring>>preconditions | selector method | method := RBParser parseMethod: source onError: [:string :position | ^RBCondition withBlock: [self refactoringError: 'The sources could not be parsed']]. selector := method selector. selector isNil ifTrue: [self refactoringError: 'Invalid source.']. ^(RBCondition definesSelector: aSelector in: aClass : selector in: class) Could you please modfify this in the package you maintain? Of course, only if you think this fix is correct! Saludos, Fernando
Hi Fernando,
Lukas, i stumbled upon a strange behavior using the refactoring engine.
When adding a method, the precondition checks if the class or any super class defines the method to be added.
It must only ask if the class directly defines it.
AddMethodRefactoring>>preconditions     | selector method |     method := RBParser parseMethod: source                 onError:                     [:string :position |                     ^RBCondition                         withBlock: [self refactoringError: 'The sources could not be parsed']].     selector := method selector.     selector isNil ifTrue: [self refactoringError: 'Invalid source.'].     ^(RBCondition definesSelector: aSelector in: aClass : selector in: class)
I don't think this is a correct change, because with this modification the AddMethodRefactoring is not a refactoring anymore. You can only add a method to a class without changing the behavior if that selector is not defined in the hierarchy. Furthermore your change breaks the unit tests, namely RBAddMethodTest. I don't know why you think this is a bug? If you use the refactoring engine to generate code then you shouldn't use the AddMethodRefactoring, but the AddMethodChange. For example to add some arbitrary methods to an arbitrary class you could use (and this is obviously not a refactoring, but some random change): | model | model := RBNamespace new. (model classNamed: #Morph) compile: 'asMorph ^ nil' classified: #(accessing). model changes open In the above example the call to #compile:classified: creates the AddMethodChange object that you then see when you open the changes log. Cheers, Lukas -- Lukas Renggli http://www.lukas-renggli.ch
You are right Lukas, what i proposed wasn't a refactoring at all! Thanks for the info and the example. Fernando On Jan 19, 2010, at 6:33 PM, Lukas Renggli wrote:
Hi Fernando,
Lukas, i stumbled upon a strange behavior using the refactoring engine.
When adding a method, the precondition checks if the class or any super class defines the method to be added.
It must only ask if the class directly defines it.
AddMethodRefactoring>>preconditions | selector method | method := RBParser parseMethod: source onError: [:string :position | ^RBCondition withBlock: [self refactoringError: 'The sources could not be parsed']]. selector := method selector. selector isNil ifTrue: [self refactoringError: 'Invalid source.']. ^(RBCondition definesSelector: aSelector in: aClass : selector in: class)
I don't think this is a correct change, because with this modification the AddMethodRefactoring is not a refactoring anymore. You can only add a method to a class without changing the behavior if that selector is not defined in the hierarchy. Furthermore your change breaks the unit tests, namely RBAddMethodTest.
I don't know why you think this is a bug? If you use the refactoring engine to generate code then you shouldn't use the AddMethodRefactoring, but the AddMethodChange. For example to add some arbitrary methods to an arbitrary class you could use (and this is obviously not a refactoring, but some random change):
| model | model := RBNamespace new. (model classNamed: #Morph) compile: 'asMorph ^ nil' classified: #(accessing). model changes open
In the above example the call to #compile:classified: creates the AddMethodChange object that you then see when you open the changes log.
Cheers, Lukas
-- Lukas Renggli http://www.lukas-renggli.ch
_______________________________________________ Pharo-project mailing list Pharo-project@lists.gforge.inria.fr http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-project
You are right Lukas, what i proposed wasn't a refactoring at all!
Thanks for the info and the example.
It's perfect that people critically look at the refactoring code. There are certainly bugs. The code base is large, highly dependent on the underlying reflective system and does quite a few non-trivial things. Just yesterday I found two subtle bugs in the refactoring model while creating tests for the OB-Refactoring integration. Lukas -- Lukas Renggli http://www.lukas-renggli.ch
participants (2)
-
Fernando olivero -
Lukas Renggli