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

[jaxrs-spec] Add builders to models #4916

Conversation

shybovycha
Copy link
Contributor

@shybovycha shybovycha commented Jan 2, 2020

As per #2415, adding builders generation for models.

The reasons for this change are:

  1. currently the setters do not have the set prefix, which makes models not comply with java beans requirements
  2. constructing models becomes painful, specifically when they have numerous fields

Compare the current way to construct a model to the proposed one:

now:

new Model().property1(value1).property2(value2)

// getters/setters

myModel.getProperty1();
myModel.property1(value1);

proposed:

Model.builder().property1(value1).property2(value2).build()

// getters/setters

myModel.getProperty1();
myModel.setProperty1(value1);

PR checklist

  • Read the contribution guidelines.
  • If contributing template-only or documentation-only changes which will change sample output, build the project before.
  • Run the shell script(s) under ./bin/ (or Windows batch scripts under.\bin\windows) to update Petstore samples related to your fix. This is important, as CI jobs will verify all generator outputs of your HEAD commit, and these must match the expectations made by your contribution. You only need to run ./bin/{LANG}-petstore.sh, ./bin/openapi3/{LANG}-petstore.sh if updating the code or mustache templates for a language ({LANG}) (e.g. php, ruby, python, etc).
  • File the PR against the correct branch: master, 4.3.x, 5.0.x. Default: master.
  • Copy the technical committee to review the pull request if your PR is targeting a particular programming language.

@shybovycha shybovycha changed the title Enhancement 4.3.x/add builders to models Add builders to models Jan 2, 2020
@shybovycha
Copy link
Contributor Author

JFYI: bitrise build is wrong - it tries to do something for swift4 with swift5-all.sh script, which apparently does not exist. Should the builds depend on the branch (e.g. swift4 for non-5.x branch and swift5 for 5.x)?

sh: bin/swift5-all.sh: No such file or directory
|                                                                              |
+---+---------------------------------------------------------------+----------+
| x | Update Swift4 samples (exit code: 127)                        | 33.32 sec|
+---+---------------------------------------------------------------+----------+
| Issue tracker: https://github.com/bitrise-io/steps-script/issues             |
| Source: https://github.com/bitrise-io/steps-script                           |
+---+---------------------------------------------------------------+----------+

@shybovycha shybovycha changed the title Add builders to models [jaxrs-spec] Add builders to models Jan 3, 2020
@wing328
Copy link
Member

wing328 commented Jan 3, 2020

The bitwise failure can be ignored as we just added the Swift 5 generator to the master.


@PUT
@Path("/body-with-query-params")
@Consumes({ "application/json" })
@ApiOperation(value = "", notes = "", tags={ "fake", })
@ApiResponses(value = {
@ApiResponse(code = 200, message = "Success", response = Void.class) })
Response testBodyWithQueryParams(@QueryParam("query") @NotNull String query,@Valid User user);
Response testBodyWithQueryParams(@QueryParam("query") @NotNull String query,@Valid User body);
Copy link
Member

Choose a reason for hiding this comment

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

I noticed that the body parameter name changes from user to body. Likely you're not using the latest (or recent) master when creating this PR.

Please merge the latest master of the official repo into your branch and regenerate the sample again.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have used 4.3.x as the origin of my branch, are you sure I should merge master in?

Copy link
Member

Choose a reason for hiding this comment

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

OK. Please put it on hold. I'll take another look over the weekend or next week.

@@ -51,6 +52,7 @@
private boolean interfaceOnly = false;
private boolean returnResponse = false;
private boolean generatePom = true;
private boolean generateBuilders = true;
Copy link
Member

@wing328 wing328 Jan 3, 2020

Choose a reason for hiding this comment

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

If this setting defaults to false, you can then target this change to the current master as it's no longer a breaking change. Otherwise, you will need to wait for 4.3.0 to be released on Feb 29.

@shybovycha shybovycha force-pushed the enhancement-4.3.x/add-builders-to-models branch from d4582ef to ccb071f Compare January 5, 2020 23:47
@shybovycha shybovycha changed the base branch from 4.3.x to master January 5, 2020 23:47
@shybovycha
Copy link
Contributor Author

To name the branch accordingly and restart the failing builds I'll close this PR and open a new one, with the correct target branch.

@shybovycha shybovycha closed this Jan 5, 2020
@shybovycha shybovycha deleted the enhancement-4.3.x/add-builders-to-models branch January 5, 2020 23:51
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.

None yet

2 participants