Re: [Pharo-dev] Generate accessors refactoring

[Denis Kudriashov]

2016-12-08 14:18 GMT+01:00 Denis Kudriashov :

> To be clear, you don't mean Do Nothing; you mean Issue an Error and Abort
>> the Refactoring right?
>
>
> No. "Do Nothing" means that if we already have method with instVar name we
> will skip this accessor. So we will not generate setter if there is already
> method #instVar:. No matter if this method do something wrong inside.


But getter will be generated in that case. No abort.

Re: [Pharo-dev] Generate accessors refactoring

[Denis Kudriashov]

Hi Eliot

2016-12-08 14:11 GMT+01:00 Eliot Miranda :

> To be clear, you don't mean Do Nothing; you mean Issue an Error and Abort
> the Refactoring right?


No. "Do Nothing" means that if we already have method with instVar name we
will skip this accessor. So we will not generate setter if there is already
method #instVar:. No matter if this method do something wrong inside.

Re: [Pharo-dev] Generate accessors refactoring

[Aliaksei Syrel]

Could we deselect check box by default in Nautilus if refactoring suggests
instVar1 / instVar1?
Every time have to go through the list and deselect...

Cheers,
Alex

On 8 December 2016 at 14:11, Eliot Miranda  wrote:

> Hi Nicolas,
>
> On Dec 8, 2016, at 1:16 AM, Nicolas Passerini 
> wrote:
>
> Let me recapitulate a little.
> A) What to do in case that a method with same selector already exists in
> the same class?
> So far I think we've come up with four ideas:
> 1. Create with an alternative name such as instVar1 / instVar1:
> 2. Create with an alternative name but ask the user what the name should
> be.
> 3. Replace existing method with simple accessor.
> 4. Do nothing.
>
>
> To be clear, you don't mean Do Nothing; you mean Issue an Error and Abort
> the Refactoring right?
>
>
> I think that the last one is the best alternative and should be the
> default action. Let me analyze the others:
> 1. Is the current behavior, but I always end up deleting the method, I
> can't think of a situation in which I want to keep a method named #name1 .
> I know that there was a lot of thinking on top of this ideas, but even so
> I think that we can (should) rethink things once in a while.
>
> 2. A little better, because lets me select a name better than #name1.
> Still I do not like it, because this means that I have a Class with
> - an instance variable 'name'
> - a method #name which is not the accessor for 'name' inst var.
> -  and an accessor which is named in another way
>
> I think this can happen in two situations:
> - The original #name method is in fact a (smarter) accessor (i.e., for
> example it provides a lazy initialization). In this case the best action is
> 4 => do nothing, accessor already exists, do not provide a new one.
> - The original #name does something else, that maybe has nothing to do
> with the variable name... in this case, I think the best is aborting the
> refactor. I am not proposing that refactoring browser should abort the
> refactor automatically, probably not, but that is what I would do as user:
> my class has a weird name clash, so I should rename either the inst var or
> the method, it has no automatic solution.
>
> I am not sure about how the "generate accessors" could help me coping with
> name clashes, but I do not like generating accessor with another name
> different from the inst var name is a good idea. We could have this as an
> option to the user, but should not be the default.
>
> 3. This is really dangerous, we shouldn't do it. I am not sure if someone
> would like to have this as a (non default) option.
>
>
> B) What if the colliding method is in a superclass?
> Well, pretty much the same but:
> - in option 3 instead of replacing a method you could override it.
> - the method in the superclass could not be a "smarter accessor" unless
> you are trying to create accessors in subclass for an inst var in the
> superclass... I do not like it. So I'd consider it allways as a name clash.
>
> Then I think that the best option is still do nothing (option 4). You
> could offer to create method with another name as alternative (option 2)...
> but I still think that this kind of name "coincidences" may introduce bugs
> in the future and should be avoided.
>
> C) Colliding methods in subclasses.
> Here we have a more subtle situation because the existing method could be
> a smarter accessor, that even could have its motivation to be there (for
> example because initialization code only is useful for one subclass).
>
> I still think that doing nothing could be a safe solution, better than
> creating it with an alternative name (but we could allow it also).
>
> Here we could think also about writing the accessor, because it will not
> override preexistent behavior. But we have to think about a few situations:
> - Easy, if any of the colliding methods (they could be serveral one for
> each subclass) is not a smarter accessor... we have name clashes and we
> cannot do anything.
> - If all colliding methods are smart accessors, we could create one in the
> superclass, provided that there exists at least one subclass which does not
> provide its own implementation.
>
>
> Of course there is not precise way of determining if a preexistent method
> can be considered as a smarter accessor or not... so we only can let the
> user decide.
>
> To resume:
> - Doing nothing will be the best default as it is suitable for all cases,
> and can not do any harm.
> - Creating methods with alternative names could be optional, but shouldn't
> be default. In this case I prefer to ask the user for a name instead of
> creating one automatically.
> - Creating the method even in case of collision can be dangerous... I am
> not sure if we should even allow it as an option.
>
> 2016-12-07 0:03 GMT+01:00 Ben Coman :
>
>> How about mostly keeping the existing behaviour, but default to
>> collisions being deselected?
>> Then at least you don't 

Re: [Pharo-dev] Generate accessors refactoring

[Eliot Miranda]

Hi Nicolas,

> On Dec 8, 2016, at 1:16 AM, Nicolas Passerini  wrote:
> 
> Let me recapitulate a little.
> A) What to do in case that a method with same selector already exists in the 
> same class?
> So far I think we've come up with four ideas:
> 1. Create with an alternative name such as instVar1 / instVar1:
> 2. Create with an alternative name but ask the user what the name should be.
> 3. Replace existing method with simple accessor.
> 4. Do nothing.

To be clear, you don't mean Do Nothing; you mean Issue an Error and Abort the 
Refactoring right?

> 
> I think that the last one is the best alternative and should be the default 
> action. Let me analyze the others:
> 1. Is the current behavior, but I always end up deleting the method, I can't 
> think of a situation in which I want to keep a method named #name1 . 
> I know that there was a lot of thinking on top of this ideas, but even so I 
> think that we can (should) rethink things once in a while. 
> 
> 2. A little better, because lets me select a name better than #name1. Still I 
> do not like it, because this means that I have a Class with 
> - an instance variable 'name'
> - a method #name which is not the accessor for 'name' inst var.
> -  and an accessor which is named in another way
> 
> I think this can happen in two situations:
> - The original #name method is in fact a (smarter) accessor (i.e., for 
> example it provides a lazy initialization). In this case the best action is 4 
> => do nothing, accessor already exists, do not provide a new one.
> - The original #name does something else, that maybe has nothing to do with 
> the variable name... in this case, I think the best is aborting the refactor. 
> I am not proposing that refactoring browser should abort the refactor 
> automatically, probably not, but that is what I would do as user: my class 
> has a weird name clash, so I should rename either the inst var or the method, 
> it has no automatic solution.
> 
> I am not sure about how the "generate accessors" could help me coping with 
> name clashes, but I do not like generating accessor with another name 
> different from the inst var name is a good idea. We could have this as an 
> option to the user, but should not be the default.
> 
> 3. This is really dangerous, we shouldn't do it. I am not sure if someone 
> would like to have this as a (non default) option.
> 
> 
> B) What if the colliding method is in a superclass?
> Well, pretty much the same but:
> - in option 3 instead of replacing a method you could override it.
> - the method in the superclass could not be a "smarter accessor" unless you 
> are trying to create accessors in subclass for an inst var in the 
> superclass... I do not like it. So I'd consider it allways as a name clash.
> 
> Then I think that the best option is still do nothing (option 4). You could 
> offer to create method with another name as alternative (option 2)... but I 
> still think that this kind of name "coincidences" may introduce bugs in the 
> future and should be avoided.
> 
> C) Colliding methods in subclasses.
> Here we have a more subtle situation because the existing method could be a 
> smarter accessor, that even could have its motivation to be there (for 
> example because initialization code only is useful for one subclass).
> 
> I still think that doing nothing could be a safe solution, better than 
> creating it with an alternative name (but we could allow it also).
> 
> Here we could think also about writing the accessor, because it will not 
> override preexistent behavior. But we have to think about a few situations:
> - Easy, if any of the colliding methods (they could be serveral one for each 
> subclass) is not a smarter accessor... we have name clashes and we cannot do 
> anything.
> - If all colliding methods are smart accessors, we could create one in the 
> superclass, provided that there exists at least one subclass which does not 
> provide its own implementation.
> 
> 
> Of course there is not precise way of determining if a preexistent method can 
> be considered as a smarter accessor or not... so we only can let the user 
> decide.
> 
> To resume:
> - Doing nothing will be the best default as it is suitable for all cases, and 
> can not do any harm.
> - Creating methods with alternative names could be optional, but shouldn't be 
> default. In this case I prefer to ask the user for a name instead of creating 
> one automatically.
> - Creating the method even in case of collision can be dangerous... I am not 
> sure if we should even allow it as an option. 
> 
> 2016-12-07 0:03 GMT+01:00 Ben Coman :
>> How about mostly keeping the existing behaviour, but default to
>> collisions being deselected?
>> Then at least you don't need to search for it.
>> It minimises the chance of missing it by mistake.
>> The deselected item easily stands out to be reselected if required.
>> 
>> P.S. An additional side idea: If collision is from a 

Re: [Pharo-dev] Generate accessors refactoring

[Nicolas Passerini]

Let me recapitulate a little.
A) What to do in case that a method with same selector already exists in
the same class?
So far I think we've come up with four ideas:
1. Create with an alternative name such as instVar1 / instVar1:
2. Create with an alternative name but ask the user what the name should be.
3. Replace existing method with simple accessor.
4. Do nothing.

I think that the last one is the best alternative and should be the default
action. Let me analyze the others:
1. Is the current behavior, but I always end up deleting the method, I
can't think of a situation in which I want to keep a method named #name1 .
I know that there was a lot of thinking on top of this ideas, but even so I
think that we can (should) rethink things once in a while.

2. A little better, because lets me select a name better than #name1. Still
I do not like it, because this means that I have a Class with
- an instance variable 'name'
- a method #name which is not the accessor for 'name' inst var.
-  and an accessor which is named in another way

I think this can happen in two situations:
- The original #name method is in fact a (smarter) accessor (i.e., for
example it provides a lazy initialization). In this case the best action is
4 => do nothing, accessor already exists, do not provide a new one.
- The original #name does something else, that maybe has nothing to do with
the variable name... in this case, I think the best is aborting the
refactor. I am not proposing that refactoring browser should abort the
refactor automatically, probably not, but that is what I would do as user:
my class has a weird name clash, so I should rename either the inst var or
the method, it has no automatic solution.

I am not sure about how the "generate accessors" could help me coping with
name clashes, but I do not like generating accessor with another name
different from the inst var name is a good idea. We could have this as an
option to the user, but should not be the default.

3. This is really dangerous, we shouldn't do it. I am not sure if someone
would like to have this as a (non default) option.


B) What if the colliding method is in a superclass?
Well, pretty much the same but:
- in option 3 instead of replacing a method you could override it.
- the method in the superclass could not be a "smarter accessor" unless you
are trying to create accessors in subclass for an inst var in the
superclass... I do not like it. So I'd consider it allways as a name clash.

Then I think that the best option is still do nothing (option 4). You could
offer to create method with another name as alternative (option 2)... but I
still think that this kind of name "coincidences" may introduce bugs in the
future and should be avoided.

C) Colliding methods in subclasses.
Here we have a more subtle situation because the existing method could be a
smarter accessor, that even could have its motivation to be there (for
example because initialization code only is useful for one subclass).

I still think that doing nothing could be a safe solution, better than
creating it with an alternative name (but we could allow it also).

Here we could think also about writing the accessor, because it will not
override preexistent behavior. But we have to think about a few situations:
- Easy, if any of the colliding methods (they could be serveral one for
each subclass) is not a smarter accessor... we have name clashes and we
cannot do anything.
- If all colliding methods are smart accessors, we could create one in the
superclass, provided that there exists at least one subclass which does not
provide its own implementation.


Of course there is not precise way of determining if a preexistent method
can be considered as a smarter accessor or not... so we only can let the
user decide.

To resume:
- Doing nothing will be the best default as it is suitable for all cases,
and can not do any harm.
- Creating methods with alternative names could be optional, but shouldn't
be default. In this case I prefer to ask the user for a name instead of
creating one automatically.
- Creating the method even in case of collision can be dangerous... I am
not sure if we should even allow it as an option.

2016-12-07 0:03 GMT+01:00 Ben Coman :

> How about mostly keeping the existing behaviour, but default to
> collisions being deselected?
> Then at least you don't need to search for it.
> It minimises the chance of missing it by mistake.
> The deselected item easily stands out to be reselected if required.
>
> P.S. An additional side idea: If collision is from a superclass then
> append (Superclass>>name) to the list item, and clicking on it shows a
> code dialog to the right similar to Spotter code review pane, to make
> a review easier.
>
> cheers -ben
>
> On Tue, Dec 6, 2016 at 10:09 PM, Yuriy Tymchuk 
> wrote:
> > It shouldn’t be too intelligent. Keep the default behavior, and for each
> > method add a 3-state checkbox: create (default), 

Re: [Pharo-dev] Generate accessors refactoring

[Yuriy Tymchuk]

> On 7 Dec 2016, at 18:00, John Brant  wrote:
> 
> Since then, I’ve had several times that it has saved me from overriding a 
> method that I didn’t want to be overridden.

Was this done by creating a method with a different name or by letting you know 
that the method already exists and asking what you want to do next?

Uko

Re: [Pharo-dev] Generate accessors refactoring

[Denis Kudriashov]

2016-12-07 18:00 GMT+01:00 John Brant :

> Which of these would you consider valid setter methods for foo?
>

I understand pretty well what you are trying to explain. I just think that
generation of methods like #instVar1/#instVar1: is useless. It always break
user control flow. Simple generator should just skip existing accessors
according to "name definition" of accessors (methods with #instVar name).
Besides skipping them not makes refactoring unsafe.

Re: [Pharo-dev] Generate accessors refactoring

[stepharong]

As for my “real experience”, I wrote the accessor refactoring over 22  
years ago. Since then, I’ve had several times that it has saved me from  
overriding a method that I didn’t want to be overridden. As for saving  
me from destroying the system, it most likely has, but not in the way  
you probably expect. When I would test the refactorings, I would  
purposely try to destroy the system by doing dangerous things. If the  
system crashed, I would fix the refactorings, or add an explanation that  
the refactoring didn’t/couldn’t check for that behavior. Testing in this  
way gave us confidence when giving demos. We could allow people to yell  
out refactorings that they wanted to see performed and we were fairly  
confident that everything would keep running. Over the years, we renamed  
Object to Thing, True/False to False/True,


I would love to try these three ;)


#+ to #p:, extracted/inlined
bunches of code in String and OrderedCollection, etc. Only once in all  
of the demos did we destroy the system. We renamed a method that was  
being used by the process scheduler. We don’t try to fix running code  
with the new names so when the original method was removed, the process  
scheduler threw an error and crashed the image.


This is a cool example.
Pablo is working on updating instances during a refactoring and we should  
try.



We could have had the
rename method refactoring handle this case also, but that would have  
required some non-portable code.



John Brant



--
Using Opera's mail client: http://www.opera.com/mail/

Re: [Pharo-dev] Generate accessors refactoring

[Eliot Miranda]

On Wed, Dec 7, 2016 at 9:00 AM, John Brant 
wrote:

> > On Dec 7, 2016, at 3:44 AM, Denis Kudriashov 
> wrote:
> >
> >
> > 2016-12-06 19:29 GMT+01:00 John Brant :
> > > We could make it configurable. "Manually" accessors will be generated
> in simplified mode and related refactorings will use intelligent mode
> >
> > My personal preference would be to ask the user what to do when
> performing the accessor refactoring. If the class hierarchy already has a
> method with the name of the variable, we can look at that method and
> determine what to do. In the case of #name, we could see that it is defined
> in a superclass and ask if it is ok to override the method (likely a good
> choice, but not always so it is good to confirm with the user). If the
> class directly defines the method, and that method has a return that
> returns the variable, then we can ask if they want to use that method as
> the “accessor” even though it does some other stuff (and potential returns
> a different value). Finally, if some subclass defines the method, then we
> can ask if they want to create that method even though some subclass is
> overriding it. Of course, we could have any combination of all three too.
> If they want a different method, then we should ask what to name the method
> instead of appending a number to the name.
> >
> > Ok. You prefer tons of questions to user just to perform stupid
> accessors generation. And, please, don't say that it is rare cases. It is
> quite common which nicely shown here 18880.
> > I wonder do you have real experience when it protected you from
> destroying system? We only talk about simple accessors refactoring.
>
> Yes, I would prefer a question to doing something wrong and breaking my
> code. Part of the problem appears to be what is defined as an "accessor”.
> Accessors as defined for refactoring are simple get/set accessors (the set
> version can return or not return the set value — depending on usage). Using
> this definition of accessor we can prove things about the code. However,
> when you extend the “accessor” definition then it gets much tougher to
> prove anything.
>
> Which of these would you consider valid setter methods for foo?
>
> foo: anInteger
> (anInteger between: 1 and: 100) ifTrue: [foo := anInteger]
>
> foo: anInteger
> (anInteger between: 1 and: 100)
> ifTrue: [foo := anInteger]
> ifFalse: [self error: ‘Invalid value’]
>
> foo: anInteger
> (anInteger between: 1 and: 100)
> ifTrue: [foo := anInteger]
> ifFalse: [bar := anInteger]
>
> foo: anInteger
> foo := anInteger.
> self changed
>
> foo: anInteger
> self aboutToChange: #foo.
> foo := anInteger.
> self changed: #foo
>
> foo: anInteger
> self change: #foo to: anInteger
>
> foo: anInteger
> “do nothing”
>
> foo: anInteger
> bar := anInteger
>
> bar: anInteger
> foo := anInteger
>
> As for whether things are “rare” or not, that would depend on the usage
> and the person performing the refactoring. If you are in the habit of
> creating some accessor like methods and then doing a create accessors
> refactoring, then you will get this behavior frequently. However, if you
> are defining the class and then immediately creating the accessors, then it
> will be much less frequent.
>
> As for my “real experience”, I wrote the accessor refactoring over 22
> years ago. Since then, I’ve had several times that it has saved me from
> overriding a method that I didn’t want to be overridden. As for saving me
> from destroying the system, it most likely has, but not in the way you
> probably expect. When I would test the refactorings, I would purposely try
> to destroy the system by doing dangerous things. If the system crashed, I
> would fix the refactorings, or add an explanation that the refactoring
> didn’t/couldn’t check for that behavior. Testing in this way gave us
> confidence when giving demos. We could allow people to yell out
> refactorings that they wanted to see performed and we were fairly confident
> that everything would keep running. Over the years, we renamed Object to
> Thing, True/False to False/True, #+ to #p:, extracted/inlined bunches of
> code in String and OrderedCollection, etc. Only once in all of the demos
> did we destroy the system. We renamed a method that was being used by the
> process scheduler. We don’t try to fix running code with the new names so
> when the original method was removed, the process scheduler threw an error
> and crashed the image. We could have had the rename method refactoring
> handle this case also, but that would have required some 

Re: [Pharo-dev] Generate accessors refactoring

[John Brant]

> On Dec 7, 2016, at 3:44 AM, Denis Kudriashov  wrote:
> 
> 
> 2016-12-06 19:29 GMT+01:00 John Brant :
> > We could make it configurable. "Manually" accessors will be generated in 
> > simplified mode and related refactorings will use intelligent mode
> 
> My personal preference would be to ask the user what to do when performing 
> the accessor refactoring. If the class hierarchy already has a method with 
> the name of the variable, we can look at that method and determine what to 
> do. In the case of #name, we could see that it is defined in a superclass and 
> ask if it is ok to override the method (likely a good choice, but not always 
> so it is good to confirm with the user). If the class directly defines the 
> method, and that method has a return that returns the variable, then we can 
> ask if they want to use that method as the “accessor” even though it does 
> some other stuff (and potential returns a different value). Finally, if some 
> subclass defines the method, then we can ask if they want to create that 
> method even though some subclass is overriding it. Of course, we could have 
> any combination of all three too. If they want a different method, then we 
> should ask what to name the method instead of appending a number to the name.
> 
> Ok. You prefer tons of questions to user just to perform stupid accessors 
> generation. And, please, don't say that it is rare cases. It is quite common 
> which nicely shown here 18880.
> I wonder do you have real experience when it protected you from destroying 
> system? We only talk about simple accessors refactoring.

Yes, I would prefer a question to doing something wrong and breaking my code. 
Part of the problem appears to be what is defined as an "accessor”. Accessors 
as defined for refactoring are simple get/set accessors (the set version can 
return or not return the set value — depending on usage). Using this definition 
of accessor we can prove things about the code. However, when you extend the 
“accessor” definition then it gets much tougher to prove anything. 

Which of these would you consider valid setter methods for foo?

foo: anInteger
(anInteger between: 1 and: 100) ifTrue: [foo := anInteger]

foo: anInteger
(anInteger between: 1 and: 100) 
ifTrue: [foo := anInteger]
ifFalse: [self error: ‘Invalid value’]

foo: anInteger
(anInteger between: 1 and: 100) 
ifTrue: [foo := anInteger]
ifFalse: [bar := anInteger]

foo: anInteger
foo := anInteger.
self changed

foo: anInteger
self aboutToChange: #foo.
foo := anInteger.
self changed: #foo

foo: anInteger
self change: #foo to: anInteger

foo: anInteger
“do nothing”

foo: anInteger
bar := anInteger

bar: anInteger
foo := anInteger

As for whether things are “rare” or not, that would depend on the usage and the 
person performing the refactoring. If you are in the habit of creating some 
accessor like methods and then doing a create accessors refactoring, then you 
will get this behavior frequently. However, if you are defining the class and 
then immediately creating the accessors, then it will be much less frequent. 

As for my “real experience”, I wrote the accessor refactoring over 22 years 
ago. Since then, I’ve had several times that it has saved me from overriding a 
method that I didn’t want to be overridden. As for saving me from destroying 
the system, it most likely has, but not in the way you probably expect. When I 
would test the refactorings, I would purposely try to destroy the system by 
doing dangerous things. If the system crashed, I would fix the refactorings, or 
add an explanation that the refactoring didn’t/couldn’t check for that 
behavior. Testing in this way gave us confidence when giving demos. We could 
allow people to yell out refactorings that they wanted to see performed and we 
were fairly confident that everything would keep running. Over the years, we 
renamed Object to Thing, True/False to False/True, #+ to #p:, extracted/inlined 
bunches of code in String and OrderedCollection, etc. Only once in all of the 
demos did we destroy the system. We renamed a method that was being used by the 
process scheduler. We don’t try to fix running code with the new names so when 
the original method was removed, the process scheduler threw an error and 
crashed the image. We could have had the rename method refactoring handle this 
case also, but that would have required some non-portable code.


John Brant

Re: [Pharo-dev] Generate accessors refactoring

[Nicolai Hess]

2016-12-07 10:44 GMT+01:00 Denis Kudriashov :

>
> 2016-12-06 19:29 GMT+01:00 John Brant :
>
>> > We could make it configurable. "Manually" accessors will be generated
>> in simplified mode and related refactorings will use intelligent mode
>>
>> My personal preference would be to ask the user what to do when
>> performing the accessor refactoring. If the class hierarchy already has a
>> method with the name of the variable, we can look at that method and
>> determine what to do. In the case of #name, we could see that it is defined
>> in a superclass and ask if it is ok to override the method (likely a good
>> choice, but not always so it is good to confirm with the user). If the
>> class directly defines the method, and that method has a return that
>> returns the variable, then we can ask if they want to use that method as
>> the “accessor” even though it does some other stuff (and potential returns
>> a different value). Finally, if some subclass defines the method, then we
>> can ask if they want to create that method even though some subclass is
>> overriding it. Of course, we could have any combination of all three too.
>> If they want a different method, then we should ask what to name the method
>> instead of appending a number to the name.
>
>
> Ok. You prefer tons of questions to user just to perform stupid accessors
> generation. And, please, don't say that it is rare cases. It is quite
> common which nicely shown here 18880
> 
> .
> I wonder do you have *real* experience when it protected you from
> destroying system? We only talk about simple accessors refactoring.
>


Possible solution in inbox for case 18880. Needs a test case. As far as I
can see,  this change does not conflict with application of
abstract-variable-refactoring, but needs some testing (exising RB-Tests are
green).

Re: [Pharo-dev] Generate accessors refactoring

[Denis Kudriashov]

2016-12-06 19:29 GMT+01:00 John Brant :

> > We could make it configurable. "Manually" accessors will be generated in
> simplified mode and related refactorings will use intelligent mode
>
> My personal preference would be to ask the user what to do when performing
> the accessor refactoring. If the class hierarchy already has a method with
> the name of the variable, we can look at that method and determine what to
> do. In the case of #name, we could see that it is defined in a superclass
> and ask if it is ok to override the method (likely a good choice, but not
> always so it is good to confirm with the user). If the class directly
> defines the method, and that method has a return that returns the variable,
> then we can ask if they want to use that method as the “accessor” even
> though it does some other stuff (and potential returns a different value).
> Finally, if some subclass defines the method, then we can ask if they want
> to create that method even though some subclass is overriding it. Of
> course, we could have any combination of all three too. If they want a
> different method, then we should ask what to name the method instead of
> appending a number to the name.


Ok. You prefer tons of questions to user just to perform stupid accessors
generation. And, please, don't say that it is rare cases. It is quite
common which nicely shown here 18880

.
I wonder do you have *real* experience when it protected you from
destroying system? We only talk about simple accessors refactoring.

Re: [Pharo-dev] Generate accessors refactoring

[Ben Coman]

How about mostly keeping the existing behaviour, but default to
collisions being deselected?
Then at least you don't need to search for it.
It minimises the chance of missing it by mistake.
The deselected item easily stands out to be reselected if required.

P.S. An additional side idea: If collision is from a superclass then
append (Superclass>>name) to the list item, and clicking on it shows a
code dialog to the right similar to Spotter code review pane, to make
a review easier.

cheers -ben

On Tue, Dec 6, 2016 at 10:09 PM, Yuriy Tymchuk  wrote:
> It shouldn’t be too intelligent. Keep the default behavior, and for each
> method add a 3-state checkbox: create (default), skip, force (override).
> Also having a usage recorder would be nice to know if developers are
> actually checking the other state more often than the default one.
>
> Uko
>
> On 6 Dec 2016, at 13:03, Thierry Goubier  wrote:
>
>
>
> 2016-12-06 11:34 GMT+01:00 Denis Kudriashov :
>>
>>
>> 2016-12-06 11:28 GMT+01:00 Yuriy Tymchuk :
>>>
>>> Additionally it was annoying if a superclass has a method with the same
>>> name. For example if you have a name var and you create accessors I’d like
>>> to have actually a ’name’ getter and not ’name1’
>>
>>
>> Yes
>>
>
> If you start to make it so intelligent it requires half a dozen click to
> generate a simple accessor, then people will stop using the refactoring and
> write the code by hand...
>
> Thierry
>
>
>

Re: [Pharo-dev] Generate accessors refactoring

[stepharong]

That isn’t a refactoring. The existing methods aren’t accessors. The  
refactoring will find accessors for the variables if they exist (even  
if they have a different names).


We all know what is refactoring. Usually we don't care about safe  
aspect of it (and we have tests). We just want simple code  
transformation. I think it is exactly such case.


“We all” probably don’t agree on much. In fact, I think many people  
would argue that the “safe aspect” is a very important part of a  
refactoring. Otherwise, they shouldn’t be called refactorings, but  
should instead be called transformations. If you call something a  
“refactoring” but don’t do basic checks on its validity then that is  
just wrong.


Yes this is why we are working on proposing refactorings and  
transformations.
We are currently refactoring the refactorings to have both transformations  
and refactorings.

Re: [Pharo-dev] Generate accessors refactoring

[John Brant]

> On Dec 6, 2016, at 10:53 AM, Denis Kudriashov  wrote:
> 
> 
> 2016-12-06 17:16 GMT+01:00 John Brant :
> > For me it is too intelligent now because it is tried to create new version 
> > of accessors instead of existing one.
> > But simple logic should just analyze method name and if accessor already 
> > exists it should skip it
> 
> That isn’t a refactoring. The existing methods aren’t accessors. The 
> refactoring will find accessors for the variables if they exist (even if they 
> have a different names).
> 
> We all know what is refactoring. Usually we don't care about safe aspect of 
> it (and we have tests). We just want simple code transformation. I think it 
> is exactly such case. 

“We all” probably don’t agree on much. In fact, I think many people would argue 
that the “safe aspect” is a very important part of a refactoring. Otherwise, 
they shouldn’t be called refactorings, but should instead be called 
transformations. If you call something a “refactoring” but don’t do basic 
checks on its validity then that is just wrong.

> Also after applying override browser shows it with special icon. And user 
> could see it.

Assuming that you didn’t override something that is an essential part of the 
image. For example, try overriding the class side method #name to return nil 
and tell me how the special icon works.

> 
> New accessors can’t override an existing method, for example #name, since 
> that would change the behavior of the #name method for that object. If you 
> are suggesting not creating the accessor method for the “myInstVar ^myInstVar 
> ifNil: [#someValue]” method, then you’ll need to change the abstract variable 
> refactorings so that they no longer use the modified create accessors 
> refactoring.
> 
> We could make it configurable. "Manually" accessors will be generated in 
> simplified mode and related refactorings will use intelligent mode

My personal preference would be to ask the user what to do when performing the 
accessor refactoring. If the class hierarchy already has a method with the name 
of the variable, we can look at that method and determine what to do. In the 
case of #name, we could see that it is defined in a superclass and ask if it is 
ok to override the method (likely a good choice, but not always so it is good 
to confirm with the user). If the class directly defines the method, and that 
method has a return that returns the variable, then we can ask if they want to 
use that method as the “accessor” even though it does some other stuff (and 
potential returns a different value). Finally, if some subclass defines the 
method, then we can ask if they want to create that method even though some 
subclass is overriding it. Of course, we could have any combination of all 
three too. If they want a different method, then we should ask what to name the 
method instead of appending a number to the name. 

Currently, the extract method works similarly. If you enter an existing 
hierarchy method name, it warns you that you could be changing the behavior and 
asks if that is what you want to do. By asking the user, we can confirm that is 
what they really want to do, and also let them know of the potential problems 
that they may not have been aware of.


John Brant

Re: [Pharo-dev] Generate accessors refactoring

[Nicolai Hess]

2016-12-06 17:53 GMT+01:00 Denis Kudriashov :

>
> 2016-12-06 17:16 GMT+01:00 John Brant :
>
>> > For me it is too intelligent now because it is tried to create new
>> version of accessors instead of existing one.
>> > But simple logic should just analyze method name and if accessor
>> already exists it should skip it
>>
>> That isn’t a refactoring. The existing methods aren’t accessors. The
>> refactoring will find accessors for the variables if they exist (even if
>> they have a different names).
>>
>
> We all know what is refactoring. Usually we don't care about safe aspect
> of it (and we have tests). We just want simple code transformation. I think
> it is exactly such case.
> Also after applying override browser shows it with special icon. And user
> could see it.
>
>
>>
>> New accessors can’t override an existing method, for example #name, since
>> that would change the behavior of the #name method for that object. If you
>> are suggesting not creating the accessor method for the “myInstVar
>> ^myInstVar ifNil: [#someValue]” method, then you’ll need to change the
>> abstract variable refactorings so that they no longer use the modified
>> create accessors refactoring.
>
>
> We could make it configurable. "Manually" accessors will be generated in
> simplified mode and related refactorings will use intelligent mode
>

The whole refactoring engine is pretty well written, we should take care to
not fix the wrong places (this has been done already in the past by people
(including me) not understanding the framework well enough).

Re: [Pharo-dev] Generate accessors refactoring

[Nicolai Hess]

2016-12-06 17:16 GMT+01:00 John Brant :

>
> > On Dec 6, 2016, at 8:25 AM, Denis Kudriashov 
> wrote:
> >
> >
> > 2016-12-06 15:09 GMT+01:00 Yuriy Tymchuk :
> > It shouldn’t be too intelligent. Keep the default behavior, and for each
> method add a 3-state checkbox: create (default), skip, force (override).
> Also having a usage recorder would be nice to know if developers are
> actually checking the other state more often than the default one.
> >
> > For me it is too intelligent now because it is tried to create new
> version of accessors instead of existing one.
> > But simple logic should just analyze method name and if accessor already
> exists it should skip it
>
> That isn’t a refactoring. The existing methods aren’t accessors. The
> refactoring will find accessors for the variables if they exist (even if
> they have a different names).
>
> New accessors can’t override an existing method, for example #name, since
> that would change the behavior of the #name method for that object. If you
> are suggesting not creating the accessor method for the “myInstVar
> ^myInstVar ifNil: [#someValue]” method, then you’ll need to change the
> abstract variable refactorings so that they no longer use the modified
> create accessors refactoring. Otherwise you can change behavior. For
> example, code such as “myInstVar ifNil: [#someOtherValue]” would no longer
> be able to return #someOtherValue and would instead return #someValue.
>
> My personal preference would be to request some additional input from the
> user in these cases. I don’t know if the 3-state checkbox or something else
> would be best. Anyway, we chose adding a number to the end of the name so
> that these methods would be easy to rename since they would have a low
> likelihood of conflicting with already existing method names. For example,
> no one implements #name1 in my image.
>
> BTW, it appears that someone changed the refactoring errors to be
> information messages that are displayed at the bottom (and quickly go away
> and sometimes appear behind your browser or other windows).


Yes, this is really bad. I tried to fix this but it is not taht easy. (And
what is even worse, in the meantime some refactoring precondition were
changed to work with this behavior (Ok, I am not totally innocent, some of
that
changes are from me, because it took some time to understand how the
refactoring preconditions and refactoring errors are working)) But we
should change this to a real error handling.


> This allows someone to perform a refactoring that failed its
> preconditions. If you are not looking at the bottom of the screen when the
> message appears you think everything is fine when it isn’t. For example, it
> let me perform the abstract class variable refactoring on DependentFields
> in Object (which breaks your image). Also, since the refactorings were
> written so that an error stopped the refactoring from running, you can now
> get debuggers when some future step of the refactoring is being executed
> when it should be executed. For example, try extracting some code with a
> return that isn’t the last part of a method:
>
> foo
> self someTest ifTrue: [ ^true ].
> ^false
>
> If you try to extract the first line, you get an information notice at the
> bottom that you can’t extract the code since it has a return, but the
> extract method keeps running, and you get a debugger when the refactoring
> is being performed on something that failed the preconditions.
>
>
> John Brant
>

Re: [Pharo-dev] Generate accessors refactoring

[stepharong]

For me it is too intelligent now because it is tried to create new  
version of accessors instead of existing one.
But simple logic should just analyze method name and if accessor  
already exists it should skip it


That isn’t a refactoring. The existing methods aren’t accessors. The  
refactoring will find accessors for the variables if they exist (even if  
they have a different names).


We all know what is refactoring. Usually we don't care about safe aspect  
of it (and we have tests). We just want simple code transformation. I  
think it is exactly such case.


+ 1

Often I get a popup telling me something that I do not understand and I  
ended doing all the changes.

So if the situation is
refactoring or doit by hand
I prefer automatic transformations under my responsibilities because  
else I have to do it by hand.


I like the idea of the radio buttons to control the behavior.

Also after applying override browser shows it with special icon. And  
user could see it.




New accessors can’t override an existing method, for example #name,  
since that would change the behavior of the #name method for that  
object. If you are suggesting not creating the accessor >>method for  
the “myInstVar ^myInstVar ifNil: [#someValue]” method, then you’ll need  
to change the abstract variable refactorings so that they no longer use  
the modified create accessors >>refactoring.


We could make it configurable. "Manually" accessors will be generated in  
simplified mode and related refactorings will use intelligent mode




--
Using Opera's mail client: http://www.opera.com/mail/

Re: [Pharo-dev] Generate accessors refactoring

[Denis Kudriashov]

2016-12-06 17:16 GMT+01:00 John Brant :

> > For me it is too intelligent now because it is tried to create new
> version of accessors instead of existing one.
> > But simple logic should just analyze method name and if accessor already
> exists it should skip it
>
> That isn’t a refactoring. The existing methods aren’t accessors. The
> refactoring will find accessors for the variables if they exist (even if
> they have a different names).
>

We all know what is refactoring. Usually we don't care about safe aspect of
it (and we have tests). We just want simple code transformation. I think it
is exactly such case.
Also after applying override browser shows it with special icon. And user
could see it.


>
> New accessors can’t override an existing method, for example #name, since
> that would change the behavior of the #name method for that object. If you
> are suggesting not creating the accessor method for the “myInstVar
> ^myInstVar ifNil: [#someValue]” method, then you’ll need to change the
> abstract variable refactorings so that they no longer use the modified
> create accessors refactoring.


We could make it configurable. "Manually" accessors will be generated in
simplified mode and related refactorings will use intelligent mode

Re: [Pharo-dev] Generate accessors refactoring

[John Brant]

> On Dec 6, 2016, at 8:25 AM, Denis Kudriashov  wrote:
> 
> 
> 2016-12-06 15:09 GMT+01:00 Yuriy Tymchuk :
> It shouldn’t be too intelligent. Keep the default behavior, and for each 
> method add a 3-state checkbox: create (default), skip, force (override). Also 
> having a usage recorder would be nice to know if developers are actually 
> checking the other state more often than the default one.
> 
> For me it is too intelligent now because it is tried to create new version of 
> accessors instead of existing one. 
> But simple logic should just analyze method name and if accessor already 
> exists it should skip it

That isn’t a refactoring. The existing methods aren’t accessors. The 
refactoring will find accessors for the variables if they exist (even if they 
have a different names). 

New accessors can’t override an existing method, for example #name, since that 
would change the behavior of the #name method for that object. If you are 
suggesting not creating the accessor method for the “myInstVar ^myInstVar 
ifNil: [#someValue]” method, then you’ll need to change the abstract variable 
refactorings so that they no longer use the modified create accessors 
refactoring. Otherwise you can change behavior. For example, code such as 
“myInstVar ifNil: [#someOtherValue]” would no longer be able to return 
#someOtherValue and would instead return #someValue.

My personal preference would be to request some additional input from the user 
in these cases. I don’t know if the 3-state checkbox or something else would be 
best. Anyway, we chose adding a number to the end of the name so that these 
methods would be easy to rename since they would have a low likelihood of 
conflicting with already existing method names. For example, no one implements 
#name1 in my image.

BTW, it appears that someone changed the refactoring errors to be information 
messages that are displayed at the bottom (and quickly go away and sometimes 
appear behind your browser or other windows). This allows someone to perform a 
refactoring that failed its preconditions. If you are not looking at the bottom 
of the screen when the message appears you think everything is fine when it 
isn’t. For example, it let me perform the abstract class variable refactoring 
on DependentFields in Object (which breaks your image). Also, since the 
refactorings were written so that an error stopped the refactoring from 
running, you can now get debuggers when some future step of the refactoring is 
being executed when it should be executed. For example, try extracting some 
code with a return that isn’t the last part of a method:

foo
self someTest ifTrue: [ ^true ].
^false

If you try to extract the first line, you get an information notice at the 
bottom that you can’t extract the code since it has a return, but the extract 
method keeps running, and you get a debugger when the refactoring is being 
performed on something that failed the preconditions.


John Brant

Re: [Pharo-dev] Generate accessors refactoring

[Denis Kudriashov]

2016-12-06 15:09 GMT+01:00 Yuriy Tymchuk :

> It shouldn’t be too intelligent. Keep the default behavior, and for each
> method add a 3-state checkbox: create (default), skip, force (override).
> Also having a usage recorder would be nice to know if developers are
> actually checking the other state more often than the default one.


For me it is too intelligent now because it is tried to create new version
of accessors instead of existing one.
But simple logic should just analyze method name and if accessor already
exists it should skip it

Re: [Pharo-dev] Generate accessors refactoring

[Yuriy Tymchuk]

It shouldn’t be too intelligent. Keep the default behavior, and for each method 
add a 3-state checkbox: create (default), skip, force (override). Also having a 
usage recorder would be nice to know if developers are actually checking the 
other state more often than the default one.

Uko

> On 6 Dec 2016, at 13:03, Thierry Goubier  wrote:
> 
> 
> 
> 2016-12-06 11:34 GMT+01:00 Denis Kudriashov  >:
> 
> 2016-12-06 11:28 GMT+01:00 Yuriy Tymchuk  >:
> Additionally it was annoying if a superclass has a method with the same name. 
> For example if you have a name var and you create accessors I’d like to have 
> actually a ’name’ getter and not ’name1’
> 
> Yes
> 
> 
> If you start to make it so intelligent it requires half a dozen click to 
> generate a simple accessor, then people will stop using the refactoring and 
> write the code by hand...
> 
> Thierry
>  
> 

Re: [Pharo-dev] Generate accessors refactoring

[Thierry Goubier]

2016-12-06 11:34 GMT+01:00 Denis Kudriashov :

>
> 2016-12-06 11:28 GMT+01:00 Yuriy Tymchuk :
>
>> Additionally it was annoying if a superclass has a method with the same
>> name. For example if you have a name var and you create accessors I’d like
>> to have actually a ’name’ getter and not ’name1’
>
>
> Yes
>
>
If you start to make it so intelligent it requires half a dozen click to
generate a simple accessor, then people will stop using the refactoring and
write the code by hand...

Thierry

Re: [Pharo-dev] Generate accessors refactoring

[Denis Kudriashov]

2016-12-06 11:28 GMT+01:00 Yuriy Tymchuk :

> Additionally it was annoying if a superclass has a method with the same
> name. For example if you have a name var and you create accessors I’d like
> to have actually a ’name’ getter and not ’name1’


Yes

Re: [Pharo-dev] Generate accessors refactoring

[Yuriy Tymchuk]

Additionally it was annoying if a superclass has a method with the same name. 
For example if you have a name var and you create accessors I’d like to have 
actually a ’name’ getter and not ’name1’

Uko

> On 6 Dec 2016, at 11:16, Nicolai Hess  wrote:
> 
> 
> 
> 2016-12-06 11:09 GMT+01:00 Denis Kudriashov  >:
> Hi.
> 
> There is very annoying logic of accessors refactoring. 
> For example imaging your class already has getter for variable #myInstVar but 
> with some extra code instead of simple return. For example it could be lazy 
> initialization:
> 
> MyClass>>myInstVar
>^myInstVar ifNil: [#someValue]
> 
> In that case accessors generator will suggest you new getter method 
> #myInstVar1. 
> I really doubt that anybody accept generation of such method. 
> I always remove it from changes list which is very annoying.
> 
> Am I alone about it? Can we remove this logic?
> 
> Best regards,
> Denis
> 
> 18880 
> 
>  Autogenerating accessors should be less naive

Re: [Pharo-dev] Generate accessors refactoring

[Nicolai Hess]

2016-12-06 11:09 GMT+01:00 Denis Kudriashov :

> Hi.
>
> There is very annoying logic of accessors refactoring.
> For example imaging your class already has getter for variable #myInstVar
> but with some extra code instead of simple return. For example it could be
> lazy initialization:
>
>
> MyClass>>myInstVar
>^myInstVar ifNil: [#someValue]
>
>
> In that case accessors generator will suggest you new getter method
> #myInstVar*1. *
> I really doubt that anybody accept generation of such method.
> I always remove it from changes list which is very annoying.
>
> Am I alone about it? Can we remove this logic?
>
> Best regards,
> Denis
>

18880

Autogenerating accessors should be less naive

Re: [Pharo-dev] Generate accessors refactoring

[Aliaksei Syrel]

+1
super annoying!

On Dec 6, 2016 11:10, "Denis Kudriashov"  wrote:

> Hi.
>
> There is very annoying logic of accessors refactoring.
> For example imaging your class already has getter for variable #myInstVar
> but with some extra code instead of simple return. For example it could be
> lazy initialization:
>
>
> MyClass>>myInstVar
>^myInstVar ifNil: [#someValue]
>
>
> In that case accessors generator will suggest you new getter method
> #myInstVar*1. *
> I really doubt that anybody accept generation of such method.
> I always remove it from changes list which is very annoying.
>
> Am I alone about it? Can we remove this logic?
>
> Best regards,
> Denis
>

[Pharo-dev] Generate accessors refactoring

[Denis Kudriashov]

Hi.

There is very annoying logic of accessors refactoring.
For example imaging your class already has getter for variable #myInstVar
but with some extra code instead of simple return. For example it could be
lazy initialization:


MyClass>>myInstVar
   ^myInstVar ifNil: [#someValue]


In that case accessors generator will suggest you new getter method
#myInstVar*1. *
I really doubt that anybody accept generation of such method.
I always remove it from changes list which is very annoying.

Am I alone about it? Can we remove this logic?

Best regards,
Denis