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): Indicate if method param is required #762

Merged
merged 2 commits into from
Sep 9, 2019

Conversation

dagnir
Copy link
Contributor

@dagnir dagnir commented Sep 3, 2019

This commit enhances javadoc generation for Java classes so that required
method/constructor parameters are indicated in their description in the javadoc.

This makes it easy for customers to see which paramaters must be provided when
interacting with libraries from Java.

Fixes #365


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

@dagnir dagnir requested a review from a team as a code owner September 3, 2019 23:50
@bmaizels
Copy link
Contributor

bmaizels commented Sep 4, 2019

Looks like you removed 'the value to be set' from builder setters. I think we only wanted to remove them from method parameters and constructor args?

@eladb
Copy link
Contributor

eladb commented Sep 5, 2019

Please fix PR title and add rationale in description

@dagnir dagnir changed the title feat(java): when generating builders indicate for each property if it… feat(java): Indicate if method param is required Sep 5, 2019
@dagnir
Copy link
Contributor Author

dagnir commented Sep 5, 2019

@eladb Done

@bmaizels bmaizels added the pr/do-not-merge This PR should not be merged at this time. label Sep 6, 2019
bmaizels
bmaizels previously approved these changes Sep 6, 2019
@bmaizels
Copy link
Contributor

bmaizels commented Sep 6, 2019

Looks good. I've disabled auto-merging so we can get a core member of the CDK team to approve it too.

@mergify mergify bot dismissed bmaizels’s stale review September 6, 2019 18:37

Pull request has been modified.

This commit enhances javadoc generation for Java classes so that required
method/constructor parameters are indicated in their description in the javadoc.

This makes it easy for customers to see which paramaters must be provided when
interacting with libraries from Java.

Fixes aws#365
eladb
eladb previously approved these changes Sep 8, 2019
@@ -19,6 +19,8 @@ protected DeprecatedClass(final software.amazon.jsii.JsiiObject.InitializationMo

/**
* @deprecated this constructor is "just" okay
* @param readonlyString This parameter is required.
* @param mutableNumber
Copy link
Contributor

Choose a reason for hiding this comment

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

shouldn't this say "the value to be set"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, per previous comment from @bmaizels, we only want that description for params on builder setters, not constructors/class methods.

@spfink spfink removed the pr/do-not-merge This PR should not be merged at this time. label Sep 9, 2019
@mergify mergify bot dismissed eladb’s stale review September 9, 2019 20:02

Pull request has been modified.

@mergify
Copy link
Contributor

mergify bot commented Sep 9, 2019

Thank you for contributing! ❤️ I will now look into making sure the PR is up-to-date, then proceed to try and merge it!

@mergify mergify bot added the pr/ready-to-merge This PR is ready to be merged. label Sep 9, 2019
@mergify mergify bot merged commit cb7e11f into aws:master Sep 9, 2019
@mergify mergify bot removed the pr/ready-to-merge This PR is ready to be merged. label Sep 9, 2019
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.

java: when generating builders indicate for each property if it's required in the docs
5 participants