2016-12-06 17:16 GMT+01:00 John Brant <br...@refactoryworkers.com>: > > > On Dec 6, 2016, at 8:25 AM, Denis Kudriashov <dionisi...@gmail.com> > wrote: > > > > > > 2016-12-06 15:09 GMT+01:00 Yuriy Tymchuk <yuriy.tymc...@me.com>: > > 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 >
