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

Remove deprecations 5.0 #6060

Merged
merged 19 commits into from
May 31, 2020
Merged

Remove deprecations 5.0 #6060

merged 19 commits into from
May 31, 2020

Conversation

jimschubert
Copy link
Member

@jimschubert jimschubert commented Apr 27, 2020

This removes the normalized/duplicated vendor extensions and warnings prepping users for the removal in 5.0. This will be a breaking change for any users with custom templates targeting any incorrectly cased vendor extensions.

This also changes from systemProperties to globalProperties and updates docs explaining which properties are available and the ways in which a user could apply these properties. This will be a breaking change for users passing -D after the generate command in the CLI, and will require Gradle users to rename the variable from systemProperties to globalProperties. Maven users defining systemProperties within the configuration node will also have to rename to globalProperties.

closes #4976

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.

cc @OpenAPITools/generator-core-team

Copy link
Contributor

@ybelenko ybelenko left a comment

Choose a reason for hiding this comment

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

I've reviewed PR locally, all works as expected.
What about -Dcolor and -Dlog.level? Shouldn't they be moved to --global-property?

@@ -29,10 +29,10 @@

import static org.openapitools.codegen.utils.StringUtils.underscore;

@SuppressWarnings("unchecked")
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't know what this suppress does, but hope it's for good reasons.

Copy link
Member Author

Choose a reason for hiding this comment

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

In Java, generic types are "erased" at runtime, so if you cast like a Map<String, Object> to Map<String, CodegenModel>, some IDEs like IntelliJ will warn that the cast is unchecked. You should also see these warnings at compilation time. You'll see these unchecked suppressions throughout our code because much of our ADT is Object typed and we extract into the concrete types. It's safe throughout because we manage the types privately, although it could fail if someone were to set a different type in an overridden method (but that would break everything anyway).

I'm hoping to improve our ADT and template binding models for 5.0 to be typed better.

@@ -271,12 +271,11 @@ public void processOpts() {
modelDescription = (modelDescription == null || modelDescription.isEmpty()) ? commentExtra : modelDescription + ". " + commentExtra;
}

if (modelVendorExtensions.containsKey(CODEGEN_VENDOR_EXTENSION_KEY)) {
if (modelVendorExtensions.containsKey(VENDOR_EXTENSION_MYSQL_SCHEMA)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

CODEGEN_VENDOR_EXTENSION_KEY vs VENDOR_EXTENSION_MYSQL_SCHEMA?
What difference does it make? 😄
Forget it, already done.

Copy link
Member Author

Choose a reason for hiding this comment

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

As a static field, this property has to make sense to consumers not only within this project but for those extending the artifact.

MysqlSchemaCodegen.CODEGEN_VENDOR_EXTENSION_KEY doesn't explain what this key is. Although it's technically accurate, you're able to have any number of vendor extensions. The new name removes the redundant CODEGEN_ prefix and changes KEY which provides no details about the public field to MYSQL_SCHEMA which represents what the vendor extension is (x-mysql-schema).

@@ -335,15 +334,15 @@ public void processIntegerTypeProperty(CodegenModel model, CodegenProperty prope
String description = property.getDescription();
String minimum = property.getMinimum();
String maximum = property.getMaximum();
Boolean exclusiveMinimum = property.getExclusiveMinimum();
Boolean exclusiveMaximum = property.getIExclusiveMaximum();
boolean exclusiveMinimum = property.getExclusiveMinimum();
Copy link
Contributor

Choose a reason for hiding this comment

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

Didn't know that Java scalars shouldn't start with capital. Maybe you need to describe it somewhere in contributor guide. When created my first PR I used AbstractPhpCodegen as reference and it filled up with String declarations. Or maybe we need some Java linter to avoid this style changes in future.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is called boxing in Java. It'll be caught by Sonar, we just have low stats on Sonar at the moment and don't make it known that we track there.

In the past couple of months, we've also introduced some quality checks in build which would warn on boxing. I think reviewers just need to be diligent on quality review.

That's not to say this is wrong. A Boolean in Java is an object that wraps a primitive boolean. Its usually frowned upon in performance critical paths, while in others it makes the most sense because of the true/false/null triple states. It should flag as a warning in most IDEs for a line like this one as unnecessary boxing, because the method you're calling returns boolean and there's no possibility of that third null state which would represent "unset".

@@ -464,8 +461,8 @@ public void processDecimalTypeProperty(CodegenModel model, CodegenProperty prope
} else {
Float min = (minimum != null) ? Float.valueOf(minimum) : null;
Float max = (maximum != null) ? Float.valueOf(maximum) : null;
if (exclusiveMinimum == true && min != null) min += 1;
if (exclusiveMaximum == true && max != null) max -= 1;
if (exclusiveMinimum && min != null) min += 1;
Copy link
Contributor

Choose a reason for hiding this comment

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

I used long declaration for readability, but it's ok. Don't like shortcuts in conditions because it's hard to read long if statements like if (!isPropValid() && !countErrors() && !$someVar){}.

@@ -1139,7 +1131,7 @@ public String escapeUnsafeCharacters(String input) {
*/
public void setDefaultDatabaseName(String databaseName) {
String escapedName = toDatabaseName(databaseName);
if (escapedName.equals(databaseName) == false) {
if (!escapedName.equals(databaseName)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Same done for readability here, but it's ok.

@jimschubert
Copy link
Member Author

What about -Dcolor and -Dlog.level? Shouldn't they be moved to --global-property?

These are not Generator properties, they're logger properties. The only thing that goes in --global-property are key/value pairs that maintain generation functionality, where the color options are not "generation logic".

Copy link
Contributor

@richardwhiuk richardwhiuk left a comment

Choose a reason for hiding this comment

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

Mostly fine, just one thing needs fixing, where the change has introduced a bug in handling of callback parameters.

@wing328 wing328 changed the base branch from 5.0.x to master May 7, 2020 06:33
@wing328
Copy link
Member

wing328 commented May 7, 2020

I've updated this PR to target the master instead as the master is now 5.0.0-SNAPSHOT

* master: (71 commits)
  [PS] check if JSON properties is defined (#6419)
  Add C++ UE4  client generator (#6399)
  Add a link to the article in dev.to (#6421)
  typescript-axios anytype is not defined (#6335)
  [Java][jersey2] Make (de)serialization work for oneOf models, add convenience and comparison methods (#6323)
  Migrate OCaml petstore to use OAS v3 spec (#6348)
  [Python-experimental] Fix type error if oneof/anyof child schema is null type (#6387)
  [Python-server] Fix blueplanet 'file not found' error  (#6411)
  [nodejs] Fix deprecation notice when running sample nodejs script (#6412)
  [java-jersey2] Conditionally include http signature mustache template (#6413)
  [bug] Fix path provider bug on CI (#6409)
  decomission nodejs server generator (#6406)
  [Java] Generate valid code if no Authentication implementations present (#5788)
  update java jersey2 samples
  [Java] Fix mustache tag in pom template for HTTP signature (#6404)
  [Python-experimental] Rename from_server variable to json_variable_naming (#6390)
  Add a link to medium blog post (#6403)
  Clean up debug in test (#6398)
  readding bin/swift5-petstore-readonlyProperties.json
  remove ./bin/swift5-petstore-readonlyProperties.json
  ...
* master:
  [kotlin][client] add support for coroutines with OkHttp (#6362)
  update package-json.lock (#6430)
  fix hardcoded match type (#6431)
  java jersey2 enhance anyOf (#6420)
  [Java][jersey2] minor improvement to jersey2 tests (#6418)
  ps minor style change (#6424)
  [JS] mark ES5 as deprecated (#6408)
  [ci] Execute maven and verify with no-snapshot-updates (#6415)
  add new file in ts axios samples
  Migrate all scala generators to use OAS3 (#6407)
  migrate ruby samples to oas3 (#6414)
@jimschubert
Copy link
Member Author

Pinging @OpenAPITools/generator-core-team again for review. I'd like to get this into the first 5.0.0 beta release.

@wing328
Copy link
Member

wing328 commented May 29, 2020

Can you please resolve the merge conflicts when you've time? I will review over the weekend. Thanks.

* master: (36 commits)
  Improve handling spaces in example command (#6482)
  fix maven plugin snapshot version
  comment out erlang server test (#6499)
  Migrate Perl samples to use OAS v3 spec (#6490)
  [core] Refactor templating management (#6357)
  migrate apex samples to use oas3 spec (#6488)
  add new file in php-symfony sample
  [PS] Refactor the http signing auth with ecdsa support (#6397)
  [Rust Server] Hyper 0.13 + Async/Await support (#6244)
  [Rust] set supportAsync to true as the default (#6480)
  [php-symfony] Set required PHP version ^7.1.3 (#6181)
  update doc
  [csharp] Rename netstandard to netstandard1.3 (#6460)
  feat: support deprecated parameters for typescript-axios generator (#6475)
  [REQ][typescript-axios] useSingleRequestParameter should mark parameter optional if all properties are optional (#6477)
  better struct alias in rust (#6470)
  Migrate Go server samples to OAS 3 only (#6471)
  [Rust][reqwest] add async support (#6464)
  [codegen][python-experimental] Composed schema with additionalProperties  (#6290)
  [Java] Decommission Retrofit 1.x support (#6447)
  ...
@jimschubert
Copy link
Member Author

Can you please resolve the merge conflicts when you've time? I will review over the weekend. Thanks.

done

@wing328 wing328 merged commit 7e5f720 into master May 31, 2020
@wing328 wing328 deleted the remove-deprecations-5.0 branch May 31, 2020 11:14
jimschubert added a commit that referenced this pull request Jun 2, 2020
* master:
  Update Generate.java (#6515)
  Undo PR #6451 (#6514)
  Minor enhancement to Python client generator's code format (#6510)
  [python-experimental] Quicken package loading (#6437)
  [Python][Client] Fix delimiter collision issue #5981 (#6451)
  [Java][Jersey2] add petstore integration tests (#6508)
  UE4 client generator fixes (#6438)
  Fix docs typos (#6478)
  [php-laravel] Show required PHP version in docs (#6502)
  [php-lumen] Show required PHP version in docs (#6501)
  [Java][Jersey2] Fix typo and script, Log enhancements, HTTP signature, deserialization (#6476)
  Remove deprecations 5.0 (#6060)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[REQ] Standardize vendor extension naming conventions
4 participants