AddMethodRefactoring>>preconditions

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
4 messages Options
Reply | Threaded
Open this post in threaded view
|

AddMethodRefactoring>>preconditions

Fernando olivero
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


_______________________________________________
Pharo-project mailing list
[hidden email]
http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-project
Reply | Threaded
Open this post in threaded view
|

Re: AddMethodRefactoring>>preconditions

Lukas Renggli
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
[hidden email]
http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-project
Reply | Threaded
Open this post in threaded view
|

Re: [Pharo-project] AddMethodRefactoring>>preconditions

Fernando olivero
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
> [hidden email]
> http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-project


_______________________________________________
Pharo-project mailing list
[hidden email]
http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-project
Reply | Threaded
Open this post in threaded view
|

Re: [Pharo-project] AddMethodRefactoring>>preconditions

Lukas Renggli
> 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

_______________________________________________
Pharo-project mailing list
[hidden email]
http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-project