-
Notifications
You must be signed in to change notification settings - Fork 245
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
feat(java): detect & rename members named after reserved words #705
Conversation
…ds found in properties and method names to avoid compiler errors
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.
Would advise adding proactive collision checks, so we bail out with an explicit error message instead of generating code that won't build...
} | ||
|
||
if (JavaGenerator.RESERVED_KEYWORDS.includes(propertyName)) { | ||
return `${propertyName}Value`; |
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.
Might be worth checking whether this is the name of an existing property on the same type... If that is so, just bail out saying you're out of options 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.
Difficult to do, so in the interests of unblocking CDK builds moving ahead without this functionality for now.
} | ||
|
||
if (JavaGenerator.RESERVED_KEYWORDS.includes(methodName)) { | ||
return `do${toPascalCase(methodName)}`; |
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.
Same as above, would be nice to have a collision check.
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.
Difficult to do, so in the interests of unblocking CDK builds moving ahead without this functionality for now.
…s a property called 'result'. Minor typo in comment fixed.
Looks good - just waiting for a confirmation build of the CDK to succeed before I approve this... To be safe... |
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.