Skip to content
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

Fixes for Issues #43 and #44 #76

Merged
merged 2 commits into from
Jul 13, 2020
Merged

Fixes for Issues #43 and #44 #76

merged 2 commits into from
Jul 13, 2020

Conversation

jfdenise
Copy link
Contributor

@jfdenise jfdenise commented Jun 23, 2020

Constant string transformation (#44) and on fix for Annotation Element value of type String (#43).

A user would define a master index such as:
org/jboss/as/jaxrs/JaxrsAnnotations$Constants.class=jaxrs-annotations.properties
org/apache/jasper/compiler/Generator.class=jsp-compiler.properties
org/jboss/as/weld/CdiAnnotations$Constants.class=cdi-annotations.properties

Each properties file would then contain specific mapping for the class.
Note that the transformation also looks inside complex String to replace parts.

Added unit test.

Signed-off-by: JF Denise jdenise@redhat.com

@tbitonti
Copy link
Contributor

Hi; reviewed the initial update. Looks fine. I have a number of comments, above, but all of the comments are for minor issues.

@jfdenise
Copy link
Contributor Author

jfdenise commented Jul 3, 2020

@tbitonti , thank-you for your review I fixed them all. I have also added a unit test.

I am wandering if the count of constant changes is correct. We are counting it both in UTF8 and in ConstantString, so that doubles the number. I checked with Direct String and it is the same.
I added a comment in the test assertion.

@tbitonti
Copy link
Contributor

tbitonti commented Jul 3, 2020

@bjhargrave This looks good to me. Can you take a quick look before merging?

Copy link
Member

@bjhargrave bjhargrave left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good but I have a few requested changes. Thanks!

Signed-off-by: JF Denise <jdenise@redhat.com>
@jfdenise jfdenise force-pushed the wf-fixes branch 2 times, most recently from 93a8972 to cfc6eb5 Compare July 13, 2020 09:35
Signed-off-by: JF Denise <jdenise@redhat.com>
@bjhargrave bjhargrave merged commit 8480ba1 into eclipse:main Jul 13, 2020
@bjhargrave
Copy link
Member

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Direct string replacement per class Annotation String parameter value not transformed
3 participants