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

@Newify table name pattern support #686

Closed
wants to merge 58 commits into from

Conversation

mgroovy
Copy link
Contributor

@mgroovy mgroovy commented Apr 17, 2018

Added class name pattern parameter support to @Newify annotation (e.g. @Newify(pattern=/[A-Z].*/)) that allows users to define the classes with newify support through the pattern in addition to the existing class list parameter.

…[reversed] is declared final but is reassigned :-)
…into bytecode)

        // 2) shouldNotCompile checks that the Groovy compiler responds in the expected way to an attempt at assigning a value to a method parameter
org.codehaus.groovy.control.MultipleCompilationErrorsException: startup failed:
TestScriptnull0.groovy: 15: The parameter [s] is declared final but is reassigned
. At [15:47]  @ line 15, column 47.
       final cls = { String s -> s = "abc";
… paramName, final List<String> classBodyTerms)
@@ -196,39 +322,10 @@ private boolean shouldTransform(DeclarationExpression exp) {
}

private boolean hasClassesToNewify() {
return classesToNewify != null && !classesToNewify.getExpressions().isEmpty();
return (classesToNewify != null && !classesToNewify.getExpressions().isEmpty()) || (classNamePattern != null); // 2018-04-07 TODO: Check if classNamePattern != null can be made more "early out"
Copy link
Contributor

Choose a reason for hiding this comment

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

I see TODO here, are you going to refine it later?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This TODO refers to e.g. an edge case optimization to defer the class name Pattern compilation to the point where is actually accessed. I don't expect this to make sense (also I am now checking against the class name pattern before checking against the class name list, since I expect this to be faster in practice), so I have not implemented it. Should probaly not be a TODO any more, but at most a regular comment, I can change this and redo the PR if you want.

if(classType.getOuterClass() != null && ((classType.getModifiers() & org.objectweb.asm.Opcodes.ACC_STATIC) == 0)) {
if(!(argsExp instanceof ArgumentListExpression)) {
addError("Non-static inner constructor arguments must be an argument list expression; pass 'this' pointer explicitely as first constructor argument otherwise.", mce);
return mce; // TODO: Expected behavior to return the untransformed method call here ?
Copy link
Contributor

Choose a reason for hiding this comment

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

Similarly, I see TODO here, are you going to refine it later? ;-)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the first Groovy extension I am doing on my own, and I took this code part from existing Groovy code, and wanted to make sure somebody more experienced had a look at this. If you say this code is fine, then the TODO can be removed (see above comment).

@daniellansun
Copy link
Contributor

daniellansun commented Apr 21, 2018

I failed to merge the PR. Could you squash the commits of the PR and create a new PR?
P.S. this is a mirror, we can not merge PR via github

FYI, #687 #688

@mgroovy
Copy link
Contributor Author

mgroovy commented Apr 21, 2018

Hi Daniel, will do (I also added two more tests (for the import alias case) and removed the TODOs).

asfgit pushed a commit that referenced this pull request Apr 22, 2018
asfgit pushed a commit that referenced this pull request Apr 22, 2018
asfgit pushed a commit that referenced this pull request Apr 22, 2018
@daniellansun
Copy link
Contributor

As the PR #689 supersedes the PR. Please close it. Thanks.
P.S. I failed to close it by myself via commit comment closes #686...

@mgroovy mgroovy closed this Apr 22, 2018
@mgroovy
Copy link
Contributor Author

mgroovy commented Apr 22, 2018

(obsolete since superseded by #689 => closing)

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.

4 participants