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

Delete unused methods in DefaultCodegen #209

Merged
merged 1 commit into from
Jun 5, 2018

Conversation

ackintosh
Copy link
Contributor

@ackintosh ackintosh commented Jun 3, 2018

PR checklist

  • Read the contribution guidelines.
  • Ran the shell script under ./bin/ to update Petstore sample so that CIs can verify the change. (For instance, only need to run ./bin/{LANG}-petstore.sh and ./bin/security/{LANG}-petstore.sh if updating the {LANG} (e.g. php, ruby, python, etc) code generator or {LANG} client's mustache templates). Windows batch files can be found in .\bin\windows\.
  • Filed the PR against the correct branch: Default: master.
  • Copied the technical committee to review the pull request if your PR is targeting a particular programming language.

Description of the PR

(details of the change, additional tests that have been done, reference to the issue for tracking, etc)

@jmini
Copy link
Member

jmini commented Jun 4, 2018

Thank you for this PR, it makes me aware of dead code.


Before removing those method, did we make an analysis about where they were used in swagger-generator v2?

For example #197 is about oneOf, maybe the method isDiscriminatorInInterfaceTree(..) could be useful (this is just a guess, I did not do an analysis yet).

@ackintosh
Copy link
Contributor Author

Sounds like a good idea.

But I think that it is better to delete the codes that is not currently in use as it is difficult to predict whether or not to use that codes. (My point is making the codes in this project more simply and keep.)

Even if we need the deleted code, it is easy to copy and use it.

What do you think?

@jmini
Copy link
Member

jmini commented Jun 5, 2018

But I think that it is better to delete the codes that is not currently in use as it is difficult to predict whether or not to use that codes. (My point is making the codes in this project more simply and keep.)

dead/unused/commented code is not a good thing... I totally agree with you.

@wing328
Copy link
Member

wing328 commented Jun 5, 2018

@ackintosh thanks for the PR to clean up unused methods.

@jmini very good point about the need to understand how these methods were used before and we can always revisit these methods when we've time by checking out a particular commit.

@wing328 wing328 merged commit b8f1e5a into OpenAPITools:master Jun 5, 2018
@wing328 wing328 added this to the 3.0.1 milestone Jun 5, 2018
@ackintosh ackintosh deleted the delete-unused-methods branch June 6, 2018 11:05
jimschubert added a commit to jimschubert/openapi-generator that referenced this pull request Jun 8, 2018
* master:
  Add 'unblu inc.' to company list (OpenAPITools#246)
  put company list in alphabetical order (OpenAPITools#244)
  [jaxrs-spec] generate spec file (yaml) correctly (OpenAPITools#243)
  [C++] Adjust the names (script, sample folder, generator) to lang option (OpenAPITools#220)
  Add GMO Pepabo to company list (OpenAPITools#242)
  [Spring] Add apiFirst option (OpenAPITools#184)
  [cli] Write to stdout/stderr, allow redirection (OpenAPITools#207)
  [JAVA][Client] New object instead of null for empty POST request (OpenAPITools#98)
  Make yaml serialization deterministic (OpenAPITools#233)
  Add syntax highlighting to migration guide (OpenAPITools#237)
  Fix shippable badge (OpenAPITools#232)
  update company list (OpenAPITools#227)
  Fix OpenAPITools#210: [Ada] Update the code generator for required and optional parameters (OpenAPITools#211)
  Delete unused methods in DefaultCodegen (OpenAPITools#209)
  add note about maven plugins (OpenAPITools#216)
  add raiffeisen to company list (OpenAPITools#223)
  add a remark about homebrew installatio (OpenAPITools#217)
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.

4 participants