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

feat(java): detect & rename members named after reserved words #705

Merged
merged 4 commits into from
Aug 14, 2019

Conversation

bmaizels
Copy link
Contributor

@bmaizels bmaizels commented Aug 13, 2019


By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

…ds found in properties and method names to avoid compiler errors
@bmaizels bmaizels requested a review from a team as a code owner August 13, 2019 21:50
Copy link
Contributor

@RomainMuller RomainMuller left a 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...

packages/jsii-pacmak/lib/targets/java.ts Outdated Show resolved Hide resolved
}

if (JavaGenerator.RESERVED_KEYWORDS.includes(propertyName)) {
return `${propertyName}Value`;
Copy link
Contributor

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...

Copy link
Contributor Author

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)}`;
Copy link
Contributor

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.

Copy link
Contributor Author

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.

@RomainMuller
Copy link
Contributor

Looks good - just waiting for a confirmation build of the CDK to succeed before I approve this... To be safe...

@RomainMuller RomainMuller changed the title feat(java): added automatic check and modification of reserved wor… feat(java): detect & rename members named after reserved words Aug 14, 2019
@RomainMuller RomainMuller merged commit 32bc117 into master Aug 14, 2019
@RomainMuller RomainMuller deleted the bmaizels/jsii-java-reserved-words branch August 14, 2019 00:32
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.

2 participants