These are chat archives for JnRouvignac/AutoRefactor

25th
Oct 2016
Luis Cruz
@luiscruz
Oct 25 2016 12:22
Hi! :)
Since I haven’t tested the rules in real apps, I might try to do it now while you go through the PR#216
For that, I believe I have to install my build as an eclipse plugin. Is that right? How can I do it?
Jean-Noël Rouvignac
@JnRouvignac
Oct 25 2016 12:38
good idea
You have a very simple solution:
Add a run configuration for an eclipse application
you can point the workspace to an existing one
or you can use a new workspace and create the projects as usual
you can either run the Eclipse application in "Run" or "Debug" mode
do you see what I mean?
Luis Cruz
@luiscruz
Oct 25 2016 12:41
Yes. It’s the first time I run an Eclipse App., but I think I got it.
Jean-Noël Rouvignac
@JnRouvignac
Oct 25 2016 12:41
Great :)
Jean-Noël Rouvignac
@JnRouvignac
Oct 25 2016 12:50
It will take me some time to review the PR, I can already see I will have quite a few comments
feel free to update the PR as I go along
Luis Cruz
@luiscruz
Oct 25 2016 12:51
:sweat_smile: ok
Jean-Noël Rouvignac
@JnRouvignac
Oct 25 2016 12:51
so I can read better :)
I like literate code
code you can read without further comments
Luis Cruz
@luiscruz
Oct 25 2016 12:51
I believe most comments will apply to the next PRs, so I will make sure I fix that before submitting them
Jean-Noël Rouvignac
@JnRouvignac
Oct 25 2016 12:52
said otherwise: never add a comment when code structure can express it
Luis Cruz
@luiscruz
Oct 25 2016 12:52
so that you don’t have so much work
Jean-Noël Rouvignac
@JnRouvignac
Oct 25 2016 12:52
I am sure there will be things in common yes
but I expected it, hell I even suggested you to go that way so you can make progress
Now is the time to do the cleanup
Luis Cruz
@luiscruz
Oct 25 2016 12:52
:)
Jean-Noël Rouvignac
@JnRouvignac
Oct 25 2016 12:53
Don't be surprised if I make comments now, then you change the code, then I make further comments, then you change the code, then I make further comments, ...
Some comments are triggered by cleaner code
Luis Cruz
@luiscruz
Oct 25 2016 12:54
no worries ;)
Jean-Noël Rouvignac
@JnRouvignac
Oct 25 2016 12:54
Because when I get out of the woods I can better see the forest
Luis Cruz
@luiscruz
Oct 25 2016 12:54
makes sense :sweat_smile:
Jean-Noël Rouvignac
@JnRouvignac
Oct 25 2016 13:39
Are you following?
Luis Cruz
@luiscruz
Oct 25 2016 13:40
I haven’t read it, I was doing the test with a real app
Jean-Noël Rouvignac
@JnRouvignac
Oct 25 2016 13:40
ok
but I’ll take a look now
Jean-Noël Rouvignac
@JnRouvignac
Oct 25 2016 13:41
ok
how did the test go?
Luis Cruz
@luiscruz
Oct 25 2016 13:42
ham, not that good. I can only make it work by using shift+alt+Y
Jean-Noël Rouvignac
@JnRouvignac
Oct 25 2016 13:42
?
Luis Cruz
@luiscruz
Oct 25 2016 13:42
and I can’t see the GUI
Jean-Noël Rouvignac
@JnRouvignac
Oct 25 2016 13:42
weird
I have never tested on MacOS though :(
Luis Cruz
@luiscruz
Oct 25 2016 13:42
but I’ve seen the GUI before
I remember the checkboxes to select the rules
Jean-Noël Rouvignac
@JnRouvignac
Oct 25 2016 13:43
if you run in debug mode, you will get the stacktraces in the console
Luis Cruz
@luiscruz
Oct 25 2016 13:43
in addition the wakelock transformation did not apply
Jean-Noël Rouvignac
@JnRouvignac
Oct 25 2016 13:43
(if any exception is thrown)
:-/
Luis Cruz
@luiscruz
Oct 25 2016 13:43
there are few exceptions indeed
Jean-Noël Rouvignac
@JnRouvignac
Oct 25 2016 13:43
welcome to my world!
Luis Cruz
@luiscruz
Oct 25 2016 13:44
:)
I can see some NPEs
I’ll have to take a deeper look into this :sweat_smile:
Jean-Noël Rouvignac
@JnRouvignac
Oct 25 2016 13:44
JDT is a bitch sometimes
Luis Cruz
@luiscruz
Oct 25 2016 13:45
hehe :laughing:
I’ll go through your review and then I’ll get back to this
Jean-Noël Rouvignac
@JnRouvignac
Oct 25 2016 13:46
I think I'll wait for a new iteration
I already made so much comments.
"so many"
Luis Cruz
@luiscruz
Oct 25 2016 13:53
yea, sounds good
Jean-Noël Rouvignac
@JnRouvignac
Oct 25 2016 14:06
The role of the ASTBuilder is to make the unreadable become readable
Jean-Noël Rouvignac
@JnRouvignac
Oct 25 2016 14:53
I have changed again the API of FinderVisitor a bit: JnRouvignac/AutoRefactor@df51fb8
Luis Cruz
@luiscruz
Oct 25 2016 14:55
Do I have to use findOrDefault?
instead of accept?
Jean-Noël Rouvignac
@JnRouvignac
Oct 25 2016 14:55
I think you should yeah
Luis Cruz
@luiscruz
Oct 25 2016 14:55
ok
Jean-Noël Rouvignac
@JnRouvignac
Oct 25 2016 14:55
it calls accepts inside
IMO it makes the code more readable when you have a finder visitor
Luis Cruz
@luiscruz
Oct 25 2016 14:56
yes, it does!
Jean-Noël Rouvignac
@JnRouvignac
Oct 25 2016 14:56
So far, are you ok with the changes I suggested?
Luis Cruz
@luiscruz
Oct 25 2016 14:56
yap
Jean-Noël Rouvignac
@JnRouvignac
Oct 25 2016 14:56
feel free to discuss if you have any issues / comments / remarks
Luis Cruz
@luiscruz
Oct 25 2016 14:57
there is one I could not understand but I skip it and I’ll get back to it. If I dont get it Ill bug you
Jean-Noël Rouvignac
@JnRouvignac
Oct 25 2016 14:57
which one? I may clarify it before you get back to it
Luis Cruz
@luiscruz
Oct 25 2016 14:59
And you need to create ASTBuilder.invoke(Expression, String, Expression... arguments)
Jean-Noël Rouvignac
@JnRouvignac
Oct 25 2016 14:59
ok
Luis Cruz
@luiscruz
Oct 25 2016 14:59
I didnt get this sentence
Jean-Noël Rouvignac
@JnRouvignac
Oct 25 2016 15:00
No method exists with this signature
so you need to create it before you can use it
Luis Cruz
@luiscruz
Oct 25 2016 15:00
but where do I need it?
Jean-Noël Rouvignac
@JnRouvignac
Oct 25 2016 15:00
the closest existing method is ASTBuilder.invoke(Expression, String, List<Expression> arguments)
when rewriting code as MethodInvocation releaseInvocation = b.invoke(b.copyExpression(methodInvocation), "release");
did you notice this? The UI is not very clear

The comment is bigger than just this:

And you need to create ASTBuilder.invoke(Expression, String, Expression... arguments)

Luis Cruz
@luiscruz
Oct 25 2016 15:01
yea yea
Jean-Noël Rouvignac
@JnRouvignac
Oct 25 2016 15:01
Do you see now?
Luis Cruz
@luiscruz
Oct 25 2016 15:02
but I have this method
public MethodInvocation invoke(Expression expression, String methodName, Expression... arguments) {
Jean-Noël Rouvignac
@JnRouvignac
Oct 25 2016 15:02
Hmmm
Luis Cruz
@luiscruz
Oct 25 2016 15:02
how is it different?
Jean-Noël Rouvignac
@JnRouvignac
Oct 25 2016 15:02
no need to create anything then :)
Luis Cruz
@luiscruz
Oct 25 2016 15:02
:P
Jean-Noël Rouvignac
@JnRouvignac
Oct 25 2016 15:02
I badly read :sweat_smile:
Luis Cruz
@luiscruz
Oct 25 2016 15:03
alright ;)
Luis Cruz
@luiscruz
Oct 25 2016 15:20
b.annotation does not exist. Did you meant other method, or it is not implemented?
Jean-Noël Rouvignac
@JnRouvignac
Oct 25 2016 15:20
You'll have to implement it :)
Luis Cruz
@luiscruz
Oct 25 2016 15:21
;)
which name do you prefer: annotation or normalAnnotation?
Jean-Noël Rouvignac
@JnRouvignac
Oct 25 2016 15:22
let me see one thing
annotation()for now
I may enhance it in the future
BTW you should use MarkerAnnotation instead of NormalAnnotation
Luis Cruz
@luiscruz
Oct 25 2016 15:25
hum, there is an ASTBuilder.normalAnnotation
public MarkerAnnotation markerAnnotation(Name typeName) {
Jean-Noël Rouvignac
@JnRouvignac
Oct 25 2016 15:26
really? Grmbl
Luis Cruz
@luiscruz
Oct 25 2016 15:26
What’s the diffference between using MarkerAnnotation and NormalAnnotation?
so just use ASTBuilder.markerAnnotation() for now
This is @Override vs. @RetentionPolicy(value=RUNTIME)
And SingleMemberAnnotation would be @RetentionPolicy(RUNTIME)
Luis Cruz
@luiscruz
Oct 25 2016 15:28
didnt know normal annotation were like @Override(). The result looked like a MarkerAnnotation

This is @Override vs. @RetentionPolicy(value=RUNTIME)
And SingleMemberAnnotation would be @RetentionPolicy(RUNTIME)

Have no idea what you’re talking about :(

Jean-Noël Rouvignac
@JnRouvignac
Oct 25 2016 15:29
lol
if you can read the grammar then you know all there is to know
A MarkerAnnotation is an annotation use where there is no parameters for the annotation (and no parentheses)
Luis Cruz
@luiscruz
Oct 25 2016 15:31
ok, finally I got it
Jean-Noël Rouvignac
@JnRouvignac
Oct 25 2016 15:31
the NormalAnnotation is the version accepting params+values
Luis Cruz
@luiscruz
Oct 25 2016 15:31
yea yea, I got that :+1:
:D
Jean-Noël Rouvignac
@JnRouvignac
Oct 25 2016 15:31
The SingleMemberAnnotation is the special case where the parameter "value" is omitted and only remain the value itself
Java oddities
Luis Cruz
@luiscruz
Oct 25 2016 15:32
yap, anyway it’s good to know this
Luis Cruz
@luiscruz
Oct 25 2016 15:42
I need help with the creation of ASTBuiilder.modifiers()
    BodyDeclaration(AST ast) {
        super(ast);
        if (ast.apiLevel >= AST.JLS3_INTERNAL) {
            this.modifiers = new ASTNode.NodeList(internalModifiers2Property());
        }
    }
in a BlockDeclaration the list of modifiers is created like that
Jean-Noël Rouvignac
@JnRouvignac
Oct 25 2016 15:42
what is that?
yeah?
Just return a List
Luis Cruz
@luiscruz
Oct 25 2016 15:43
so if I want to create a method that returns a modifiers list, it should follow this approach I believe
Jean-Noël Rouvignac
@JnRouvignac
Oct 25 2016 15:43
oh no
Luis Cruz
@luiscruz
Oct 25 2016 15:43
ah ok :sweat_smile:
Jean-Noël Rouvignac
@JnRouvignac
Oct 25 2016 15:43
public List<IEntendedModifier> modifiers()
or something like this
and just return a regular arraylist
then we will be using addAll() to the actual node list in ASTBuilder
dunno if I am clear :(
Luis Cruz
@luiscruz
Oct 25 2016 15:44
I think so
Jean-Noël Rouvignac
@JnRouvignac
Oct 25 2016 15:45
pheww
:)
Luis Cruz
@luiscruz
Oct 25 2016 15:45
:)
In this proposed implementation we’re returning a different object
How can I get a MethodDeclration from a IMethodBinding?
Jean-Noël Rouvignac
@JnRouvignac
Oct 25 2016 17:04
Wait I need a computer
:)
Luis Cruz
@luiscruz
Oct 25 2016 17:10
nice catch with the WakeLock wl; I don’t know what I was thinking
Jean-Noël Rouvignac
@JnRouvignac
Oct 25 2016 17:10
I know
do - redo -reredo cycle
and you lost the original idea :)
Luis Cruz
@luiscruz
Oct 25 2016 17:10
yea probably :sweat_smile:
Luis Cruz
@luiscruz
Oct 25 2016 17:25
I think I’ve fixed most comments.
I will update the PR to clean it a bit. Is that okay?
I’m not going to squash it yet, since this will not be the accepted version
Jean-Noël Rouvignac
@JnRouvignac
Oct 25 2016 17:32
Please go for it
Aww you need to javadoc everything
I am talking about the new methods in ASTBuilder
Luis Cruz
@luiscruz
Oct 25 2016 17:43
yea, no worries
I’ll take care of it but we can move on with the review ;)
I’ll stop now. Tomorrow I’m not sure if I’ll have time for this, but I will try in the afternoon :)
Jean-Noël Rouvignac
@JnRouvignac
Oct 25 2016 17:48
no worries
do it on your on time
I have my own work to do too :)
Vaibhav Tulsyan
@xennygrimmato
Oct 25 2016 20:29
Hey! I like this project and I would like to contribute to it. Is there an easy issue that I can fix on this repo or a cool feature that I can implement to start with? :)
Jean-Noël Rouvignac
@JnRouvignac
Oct 25 2016 20:30
Welcome Vaibhav
Vaibhav Tulsyan
@xennygrimmato
Oct 25 2016 20:30
Hello @JnRouvignac :)
Jean-Noël Rouvignac
@JnRouvignac
Oct 25 2016 20:30
Sure I can find something for you
Vaibhav Tulsyan
@xennygrimmato
Oct 25 2016 20:30
@JnRouvignac Thanks!
Jean-Noël Rouvignac
@JnRouvignac
Oct 25 2016 20:32
Have a look at #105
I think that would not be too hard to implement
Vaibhav Tulsyan
@xennygrimmato
Oct 25 2016 20:33
Interesting..
Jean-Noël Rouvignac
@JnRouvignac
Oct 25 2016 20:34
Give it a try
Do you need guidance or you do you prefer to work on your own?
You can have a look at the wiki there is already some content there
Vaibhav Tulsyan
@xennygrimmato
Oct 25 2016 20:35
I think if I understand the basic components of how such a statement is represented, it shouldn't be so hard.
Jean-Noël Rouvignac
@JnRouvignac
Oct 25 2016 20:38
Sorry I cannot assign the issue to you because I need to add you as a contributor and this gives too large permissions
I will do it gladly a bit later
Vaibhav Tulsyan
@xennygrimmato
Oct 25 2016 20:40

@JnRouvignac Can you tell me what the basic building blocks are? For example, I'll have to look for constants and variables in the AST, and then modify the AST accordingly I guess. It would be great if you can tell me where rules for the existing refactorings lie.

Oh, sure no problem - I just thought any Github developer can be assigned an issue without adding as contributor. No worries there :)

You can check the rules dealing with if statements
Jean-Noël Rouvignac
@JnRouvignac
Oct 25 2016 20:43
I suggest you start with writing samples and write the skeleton of a rule
Then run the RefactoringRuleTest
Make sure you run it as a Junit plugin test
Vaibhav Tulsyan
@xennygrimmato
Oct 25 2016 20:44
Ok
I just need to set up some stuff on my machine first - don't have eclipse working yet! Will just write some simple stuff to get my hands dirty as soon as possible :)
Jean-Noël Rouvignac
@JnRouvignac
Oct 25 2016 20:49
Yep there is a guide on the wiki to setup the environment
Jean-Noël Rouvignac
@JnRouvignac
Oct 25 2016 20:53
Great I can see you're on your way
Feel free to post comments / questions here
I will soon go to bed. I'll answer tomorrow in that case
Vaibhav Tulsyan
@xennygrimmato
Oct 25 2016 20:55
Sure :)
It's 2:25 am here, so I might go to bed in a while too, in which case I'll ask questions tomorrow :)
Jean-Noël Rouvignac
@JnRouvignac
Oct 25 2016 21:02
Sure, whatever you prefer :)
Vaibhav Tulsyan
@xennygrimmato
Oct 25 2016 21:04
Thanks!
Vaibhav Tulsyan
@xennygrimmato
Oct 25 2016 21:16
The link to Tycho Configurator Plugin seems to be incorrect @JnRouvignac
Jean-Noël Rouvignac
@JnRouvignac
Oct 25 2016 21:26
Thanks, fixed now
Vaibhav Tulsyan
@xennygrimmato
Oct 25 2016 21:27
Cool
Also, I think the latest version of Eclipse doesn't have a Maven -> Discovery option in Preferences
Jean-Noël Rouvignac
@JnRouvignac
Oct 25 2016 21:31
Neon? Ah crappie
Vaibhav Tulsyan
@xennygrimmato
Oct 25 2016 21:31
Yeah
Jean-Noël Rouvignac
@JnRouvignac
Oct 25 2016 21:31
Damn autocomplete!
:)
Vaibhav Tulsyan
@xennygrimmato
Oct 25 2016 21:31
Haha
I am trying to install Tycho
Facing trouble. Dunno how to go about it here.
Btw, the wiki states that I must use http://download.eclipse.org/eclipse/updates/4.5 for downloading PDE. Is there a specific reason for this, or can I use a newer version of PDE as well?
Jean-Noël Rouvignac
@JnRouvignac
Oct 25 2016 21:35
Instructions where written for Mars (4.5)
Vaibhav Tulsyan
@xennygrimmato
Oct 25 2016 21:35
I see
Jean-Noël Rouvignac
@JnRouvignac
Oct 25 2016 21:35
So you must use 4.6 for neon
Vaibhav Tulsyan
@xennygrimmato
Oct 25 2016 21:36
coo
cool
Jean-Noël Rouvignac
@JnRouvignac
Oct 25 2016 21:37
Updated
Vaibhav Tulsyan
@xennygrimmato
Oct 25 2016 21:38
thanks
Vaibhav Tulsyan
@xennygrimmato
Oct 25 2016 21:57
Can I start off without installing Tycho?