review: fix: Dynamic lookup issues by removing dynamic lookup #1215
Conversation
surli
changed the title
no, dynamic lookup is a core design decision that was already there at the birth of Spoon more than ten years ago |
|
Nope this lookup based on parents has been introduced 8 months ago to solve this issue #756 |
|
The dynamic lookup algorithm introduced in #756, is step in wrong direction. The problem of #756 is that client wants to rename class A to B, and it is kind of java model refactoring, which cannot be easily done by And I think that solution of this PR is not good idea too, because it tries to pretend that renaming of class is a primitive operation. What if somebody wants to rename field, variable, method, parameter, ...? Are we able to implement everything into CtNamedElementImpl ? Suggestion: May be the CtNamedElementImpl#setSimpleName should only check what kind of element we are renaming and by default throw an exception which says "Modification of model is not primitive operation. Please use refactoring methods from package ...spoon.refactor...". That exception would be thrown optionally depending on settings of spoon launcher/factory, so after clients are warned and they change that setting with idea "I know what I am doing", they can continue renaming without further warning exceptions ... |
| @@ -43,6 +49,22 @@ public String getSimpleName() { | |||
| if (factory instanceof FactoryImpl) { | |||
| simpleName = ((FactoryImpl) factory).dedup(simpleName); | |||
| } | |||
|
|
|||
| if (this instanceof CtType && !simpleName.equals("")) { | |||
There was a problem hiding this comment.
We should not change setSimpleName here. Instead we should change the CloneVisitorGenerator to call Refactor.rename().
There was a problem hiding this comment.
Refactor.rename()
I agree.
| @@ -28,18 +28,22 @@ | |||
| */ | |||
| public final class Refactoring { | |||
| /** | |||
| * Changes name of a type element. | |||
| * Changes names of the inherited reference of type qualified by oldName | |||
The issue #1213 shows that in some cases dynamic lookup just fail to find the right class. Then we propose to remove the dynamic lookup, and to improve the setName instead.
According to @tdurieux dynamic lookup was introduced to support the clone operations: we propose instead to always use renaming operation after using clone, and then to improve renaming to also rename children of a type.
A mandatory change is then needed: to use the
setNameoperation after putting a type in the package.