-
Notifications
You must be signed in to change notification settings - Fork 88
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
[MSHADE-396] Improve SourceContent Shading #105
Conversation
Hi @kriegaex I have corrected this error with the following change. // Excludes should be subpackages of the global pattern
- else if ( exclude.startsWith( pathPattern ) )
+ if ( exclude.startsWith( pathPattern ) )
{ To test, you need to apply the following lines.
SimpleRelocator relocatorPackage = new SimpleRelocator( "io", "shaded.io", null, null);
SimpleRelocator asmPackage = new SimpleRelocator( "org.objectweb.asm", "aj.org.objectweb.asm", null, null);
+ SimpleRelocator relocatorExlucdeSubPackage = new SimpleRelocator( "foo", "shaded.foo", null , Arrays.asList( "foo.bar"));
assertEquals(
relocatedFile,
- asmPackage.applyToSourceContent(
- relocatorPackage.applyToSourceContent(
- relocator.applyToSourceContent( sourceFile )
+ relocatorExlucdeSubPackage.applyToSourceContent(
+ asmPackage.applyToSourceContent(
+ relocatorPackage.applyToSourceContent(
+ relocator.applyToSourceContent( sourceFile )
+ )
)
)
); |
@rmannibucau Would you pls review this PR too? |
Improve search & replace heuristics without destroying previously correct replacements in my test project (AspectJ). This solution is still bound to fail in some situations, simply because it is just a heuristic approach and not a full Java parser correctly recognising package names in all possible situations in Java source code. As for matching within Java string constants, this is next to impossible to get 100% right. But the source shading feature is not meant as a source code generator anyway, merely as a tool creating reasonably plausible source code when navigating to a relocated library class from an IDE, hopefully displaying source code which makes 95% sense - no more, no less.
Explain purpose and limitations of heuristic source code shading approach.
231c544
to
80fa5aa
Compare
Good catch! Because your previous change did not have an obvious test case, I did not understand why my code was wrong in this corner case. I have fixed it and added your test case, then squashed the change into the first commit. Barring a positive code review, I think this is ready to be merged. |
I have checked the generated source code and found no error. PR looks good to me. |
What is blocking a merge? |
@Reamer, I am not sure, not being a Maven committer. Maybe we need more than one approved review. But no other reviewer signed up for this yet. |
@rmannibucau and @Tibor17 |
Do you want to squash commits before we merge or not (personally I don't care at all but asking before hitting merge ;))? |
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.
+1
I can not decide this. But I think that @kriegaex will quickly comment on this. |
IMO, more and smaller commits are preferable. Squashing in many cases is some kind of information hiding. But I am aware of the fact that Tibor is a decided proponent of squashing (one PR, one commit). I could not disagree more, but I guess that if he wants it squashed, he will do so anyway, like he did before with another recent PR of mine. In any case, we can all say our opinions, but in the end the person responsible for the merge has the final say and we have to respect whatever he or she does. So let's not make it a big debate. 🙂 |
Improve search & replace heuristics without destroying previously correct replacements in my test project (AspectJ). This solution is still bound to fail in some situations, simply because it is just a heuristic approach and not a full Java parser correctly recognising package names in all possible situations in Java source code. As for matching within Java string constants, this is next to impossible to get 100% right.
But the source shading feature is not meant as a source code generator anyway, merely as a tool creating reasonably plausible source code when navigating to a relocated library class from an IDE, hopefully displaying source code which makes 95% sense - no more, no less.
Supersedes #100 and fixes a few bugs regression I found with the new approach.
To make clear that you license your contribution under
the Apache License Version 2.0, January 2004
you have to acknowledge this by using the following check-box.