Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

[PHP][Laravel] Remove duplicate variable declaration #792

Merged

Conversation

jmini
Copy link
Member

@jmini jmini commented Aug 12, 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: master, 4.0.x. Default: master.
  • Copied the technical committee to review the pull request if your PR is targeting a particular programming language.

Description of the PR

Small code review: Introduced with #574, this PR fixes following error:

The field PhpLaravelServerCodegen.variableNamingConvention is hiding a field from type AbstractPhpCodegen

We decided to not use fields hiding other in child classes, because this is a bad practice in Java.

cc: @renepardon, @wing328, @ackintosh


There is an open issue (#33) to integrate a static code analysis tool to automate detection of those cases.

etherealjoy
etherealjoy approved these changes Aug 12, 2018
Copy link
Contributor

@etherealjoy etherealjoy left a comment

Choose a reason for hiding this comment

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

Please ignore this Review Approval, I had multiple tabs opened and I was actually working with #731.
:(

Copy link
Member

@wing328 wing328 left a comment

Choose a reason for hiding this comment

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

looks good and consistent with other PHP server generator

@wing328 wing328 added this to the 3.2.1 milestone Aug 13, 2018
@wing328 wing328 merged commit 5e1c4cd into OpenAPITools:master Aug 13, 2018
@wing328 wing328 changed the title Remove duplicate variable declaration [PHP][Laravel] Remove duplicate variable declaration Aug 13, 2018
jimschubert added a commit to jimschubert/openapi-generator that referenced this pull request Aug 14, 2018
* master: (32 commits)
  Fixed date formatting in typescript node client (OpenAPITools#786)
  better explain usage (OpenAPITools#794)
  Fix float/double default value in C# generator (OpenAPITools#791)
  Enhancements to documentation generators (samples, default values, etc) (OpenAPITools#790)
  Remove duplicate variable declaration (OpenAPITools#792)
  Issue 758 root resource (OpenAPITools#771)
  Do not declare destructor as default when destructor is explicitly declared. (OpenAPITools#732)
  Fix C# client enum issue (OpenAPITools#774)
  [JavaScript] Update vulnerable dependencies (OpenAPITools#784)
  [Ruby] Fix method split (OpenAPITools#780)
  [Java][jaxrs-jersey] add sample with jaxrs-jersey + openapi v3 (OpenAPITools#778)
  update groupId in pom (OpenAPITools#779)
  [cpp-restsdk] Support multi-line descriptions (OpenAPITools#753)
  [Core] Resolve Inline Models (OpenAPITools#736)
  [gradle] Support nullable system property values (OpenAPITools#764)
  Correct URL for openapi-generator.cli.sh in README.md (OpenAPITools#770)
  Fixed the generation of model properties whose data type is a composed (allOf) schema (OpenAPITools#704)
  [JAX-RS][Spec] Add samples to CircleCI (OpenAPITools#759)
  minor update to python generator usage (OpenAPITools#762)
  [C++][Restbed/Pistache] Added fix for byte array (OpenAPITools#752)
  ...
A-Joshi pushed a commit to ihsmarkitoss/openapi-generator that referenced this pull request Feb 27, 2019
# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants