New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
review: feature: add support for sniper mode #1927
Conversation
fd61958
to
0ea3ea4
Compare
b39d22a
to
10e9a2f
Compare
Any idea how to test it? There is actually |
super interesting. it seems to be simply the sniper mode? correct? for testing, I propose the following contract:
For testing the forall, we can sample random subtrees in src/main and ideally we would have a tree generator, see #1960 |
yes, is it sniper mode, but based on different concept then #1358. But similar like #1358 it is hard to implement it 100% correctly. Actually it supports printing of changes in type or method and in all other cases it falls back to normal printing. But even this is much better then nothing.
I never heard about that, but it sounds interesting. I will at least read some overview/tutorial to understand what is possible. Thanks for the tip.
the problem I see in testing is that these tests must accept some degree of incorrectness - in case of unsupported element - when it will print correct java code, but with default formatting - not with origin formatting. Also comments are not handled yet. But it looks it will be possible to handle them too. I read also the tests implemented for #1358, but there is no solution too. |
@monperrus, @surli , @tdurieux who would like to review that? I would like to discuss next steps with reviewer. Note: this PR is somehow working and fully commented, but has still many todos (like comment handling, bug fixing, tests, etc.). I see these ways how to proceed with this PR: A) to review and merge it as experimental nearly with the incomplete algorithms The case A) is nearly baby step. The case B) will produce another big PR and hard to review PR. WDYT? Note: first commit of this PR is #1950. So after that one is merged and after rebase, it will be little bit simpler too. |
Personally, I think that this type of feature can only be tested by using it. I think it is a good idea to merge like we did with the roles. |
One question, how do you handle imports? |
if (e instanceof CtPackage) { | ||
/* | ||
* do not generate SourcePosition for package-info files, because | ||
* 1) JDT compiler actually delivers 0,0 as source position of type package-info. Comments and annotations are not included |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now, the package.info position contains the complete file, I don't know if it is more useful for you
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, I will check it and may be remove that change. Thank you for information.
Personally, I struggled a lot to handle the CtCactch and CtSwitch that have really bad positions. |
I always generate them - never use origin source code. I want to focus on them later. I have not though about that deeper yet. |
This feature will be great to do automatic fixing of SonarQube violations. |
missing "sniper mode" is the show stopper for usage of spoon for refactoring. Whenever I speak with my colleagues about Spoon, they are first excited ... until they hear that any transformed and then printed code will produce many useless changes in source code. It is not acceptable for projects, where history of changes is daily needed to detect who is author of what and why it is done as it is done... |
|
||
new SourceFragmentsTreeCreatingChangeCollector().attachTo(f.getEnvironment()); | ||
//change the model | ||
ctClass.getField("string").setSimpleName("modified"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pavel, can you explain how can I add my own processor in sniper mode and how to use it on a directory with many files instead of a single class?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi Ashutosh,
Note: that this PR is experimental. I never tried it on real sources and I expect some problems which has to be solved first. So there is actually no easy way how to enable sniper mode in Spoon API. You have to do it manually in these steps:
- register one change listener
SourceFragmentsTreeCreatingChangeCollector
in spoon environment AFTER the model is loaded and BEFORE you start any changes. - do changes on your model
- print changes (type by type) using new printer
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright. Thanks a lot.
When I'm analysing this simple class(without making changes) : public class A
{
public static void pong(String args)
{
System.out.println("Hello World");
}
} everything is fine, but if I change the argument of pong to String args[] , it is showing the following error : spoon.SpoonException: The SourcePosition of elements are not consistent
parentFragment: |45, 56|String args|
otherFragment: |45, 58|String args[]|
at spoon.reflect.visitor.printer.change.SourceFragment.addChild(SourceFragment.java:265) String []args works fine though. |
Thank You for reporting that. It looks like a problem in computation of source positions. I will may be have time to have a look at that next week. I made PR #2015 with failing test for this problem. |
Rebase was needed. I hope MavenLauncherTest will pass - there were many conflicts in that file |
3058c79
to
0ac1269
Compare
I would suggest to first merge ##2461 to get rid of the problem with MavenLauncherTest and have cleaner diffs for subsequent PRs incl. this one. |
ok, I agree. No hurry is needed here |
API changes: 3 (Detected by Revapi) Old API: fr.inria.gforge.spoon:spoon-core:jar:7.1.0-20180918.153340-173 / New API: fr.inria.gforge.spoon:spoon-core:jar:7.1.0-SNAPSHOT
|
We don't want to rebase this one. Are you OK if I reset the last commit on master and push force? |
yes, I am OK with reset and push force. I have no new code there |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Champagne for the sniper mode!
Thanks a lot to @GerardPaligot for the initial work on modelobs, @tdurieux for having kept it in the attic, as well as the awesome contributions on positions and @pvojtechovsky for the final heavy-lifting.
That's impressive team work over time.
I've used javaparser for my first try with automated code refactoring and then got the hint to check out spoon. Its really great but the many useless code changes and especially the comment handling, their position is really important in my current project, are a showstopper for real code transformation. Therefore i use it only for analysis and do the plumbing then with simple regex code manipulation based on the collected information. Would be really nice to have a "minimal change impact" mode. |
This is exactly what we have now. Have you tried the sniper mode? See http://spoon.gforge.inria.fr/launcher.html ("sniper mode") |
@stefanleh and let us know your experience - you will be probably the first brave client who tries that new toy on real project ;-) |
FYI a real pull-request made with the sniper mode by @HarisAdzemovic |
Really cool! |
C'est bluffant!
Le 07/11/2019 à 18:36, Thomas Durieux a écrit :
… Salut Martin (CC: Benoit: tu devrais aussi aimer ca),
OpenAi a publié son générateur de fake news, et tu peux l’essayer
ici: https://talktotransformer.com/.
J’ai été curieux, et je voulais voir sa capacité a générer du text
pour un PR (à partir du titre), le résultat m’a pas mal surpris...
Titre: Strings and Boxed types should be compared using "equals()”
Text:
Strings and Boxed types should be compared using "equals()" rather
than "==" like most of the other comparison operators. However,
equality is more convenient when the type of both objects can be
compared in constant time.
For example, if String is an instance of the Boxed class and Integer
is an instance of the String class then a String object that is
compared with an Integer object using "==" would perform constant time
operation, whereas the "==" operator does not perform a comparison on
a String object in constant time.
If you want to compare two primitive types in constant time, you can
compare them with the "==" operator. However, this operator is rarely
used for such a comparison.
Here are some examples of comparison in constant time.
double.compare(float.compare(double.compare(int.compare(Integer)))); double.compare(double.compare(int.compare(Double.compare(Integer,
Integer
WDYT?
Thomas
> On 6 Nov 2019, at 04:16, Martin Monperrus ***@***.***
> ***@***.***>> wrote:
>
> FYI a real pull-request made with the sniper mode by @HarisAdzemovic
> <https://github.com/HarisAdzemovic>
>
> apache/sling-org-apache-sling-discovery-impl#1
> <apache/sling-org-apache-sling-discovery-impl#1>
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <#1927?email_source=notifications&email_token=ABKRWYENBK2EIW7FSE4RHITQSKDPVA5CNFSM4EXJCCJKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEDF22MQ#issuecomment-550219058>,
> or unsubscribe
> <https://github.com/notifications/unsubscribe-auth/ABKRWYGWDE4R7PU6I4FNNMDQSKDPVANCNFSM4EXJCCJA>.
>
--
Professor in Software Technology, KTH Royal Institute of Technology
https://softwarediversity.eu
Director of the CASTOR center for software research
https://www.castor.kth.se/
|
fix #1284