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.

Sign up for GitHub

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

Jump to bottom

[Slim] Improve codebase decouple #438

Merged
merged 2 commits into from
Jul 6, 2018
Merged

[Slim] Improve codebase decouple #438

merged 2 commits into from
Jul 6, 2018

Conversation

ybelenko
Copy link
Contributor

@ybelenko ybelenko commented Jul 2, 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, 3.1.x, 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

Logical prepare before unit test generation feature.
Now generated package splitted into ApiControllers, Models and SlimRouter. This provides simple approach for unit testing and code coverage.
Samples omitted in first commit to simplify code review.

cc @jebentier @dkarlovi @mandrean @jfastnacht @ackintosh

@ybelenko ybelenko mentioned this pull request Jul 3, 2018
Closed
@ybelenko ybelenko force-pushed the slim_enhancement_decouple branch 2 times, most recently from 6dd7aa6 to 81b5bc3 Compare July 5, 2018 08:32
ackintosh
ackintosh suggested changes Jul 5, 2018
View reviewed changes
...openapi-generator/src/main/java/org/openapitools/codegen/languages/PhpSlimServerCodegen.java Outdated
public void setInvokerPackage(String invokerPackage) {
super.setInvokerPackage(invokerPackage);
apiPackage = invokerPackage + "\\" + apiDirName;
modelPackage = invokerPackage + "\\" + modelDirName;
Copy link
Contributor

Choose a reason for hiding this comment

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

+        apiPackage = invokerPackage + "\\" + apiDirName;
+        modelPackage = invokerPackage + "\\" + modelDirName;

As the method name setInvokerPackage() implies, it should set only invokerPackage. 💡

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ackintosh
I've copied it from PhpSymfony generator.
https://github.com/OpenAPITools/openapi-generator/blob/master/modules/openapi-generator/src/main/java/org/openapitools/codegen/languages/PhpSymfonyServerCodegen.java#L481-L490

Where I should place these changes then?

@ybelenko
[Slim] Decouple Api files into separated PHP Classes
40f97e6 
This enhancement required for modular testing and code coverage generating.
@ybelenko
[Slim] Define all app routes in SlimRouter PHP Class. Generate new sa…
b7addcd 
…mples
@ybelenko
Copy link
Contributor Author

ybelenko commented Jul 5, 2018

Fixed. setInvokerPackage overriden method deleted.
I will try to solve Composer autoload issue in next PRs, for now user can set wrong values(when modelPackage and apiPackage doesn't start with invokerPackage Composer autoloader is broken). Maybe we can just throw a warning to user instead of fixing them when provided values are incorrect.

ackintosh
ackintosh approved these changes Jul 6, 2018
View reviewed changes
Copy link
Contributor

@ackintosh ackintosh left a comment

Choose a reason for hiding this comment

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

Thanks for the fix. Looks good to me.

@ybelenko
Copy link
Contributor Author

ybelenko commented Jul 6, 2018

Upgrade note

Current refactoring brings breaking changes. If your application stop working you need to copypaste endpoints implementation from index.php to corresponding ApiControllers(OpenAPIServer\Api\FakeApi class for instance). That simple code relocation should fix your app.

@wing328
Copy link
Member

wing328 commented Jul 6, 2018

The Shippable failure has been addressed in the master.

@wing328 wing328 added this to the 3.1.0 milestone Jul 6, 2018
@wing328 wing328 merged commit 9eeeded into OpenAPITools:master Jul 6, 2018
@ybelenko ybelenko deleted the slim_enhancement_decouple branch July 6, 2018 06:38
jimschubert added a commit to jimschubert/openapi-generator that referenced this pull request Jul 7, 2018
@jimschubert
Merge branch 'master' into generator-validation-option
232166e 
* master: (116 commits)
  3.1.0 release (OpenAPITools#486)
  Fix broken link to openapi generator plugin README (OpenAPITools#484)
  show warning message for nodejs server only (OpenAPITools#481)
  Add grokify to Go technical committee (OpenAPITools#479)
  [Slim] Improve codebase decouple (OpenAPITools#438)
  Ensure typescript samples are up to date (OpenAPITools#444)
  Update README.md
  [Golang][client] delete sample output dir before rebuild (OpenAPITools#477)
  update petstore samples (OpenAPITools#478)
  Revert "Improve Docker Tags (OpenAPITools#390)"
  update go client test dependencies (OpenAPITools#468)
  [Golang][client] fix for schema definition name `file` (OpenAPITools#433)
  Fix '.travis' file (syntax)
  make LICENSE GitHub display compatible (OpenAPITools#467)
  Improve Docker Tags (OpenAPITools#390)
  [Golang][client] fix file suffix for _test.go (OpenAPITools#449)
  Remove copy section (OpenAPITools#463)
  Add link to presentation (OpenAPITools#465)
  Use postProcessOperationsWithModels(Map, List) (OpenAPITools#431)
  [C] Adding petstore sample written in C (OpenAPITools#306)
  ...
A-Joshi pushed a commit to ihsmarkitoss/openapi-generator that referenced this pull request Feb 27, 2019
@ybelenko @wing328
[Slim] Improve codebase decouple (OpenAPITools#438)
b198c91 
* [Slim] Decouple Api files into separated PHP Classes

This enhancement required for modular testing and code coverage generating.

* [Slim] Define all app routes in SlimRouter PHP Class. Generate new samples
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ackintosh ackintosh ackintosh approved these changes

Assignees
No one assigned
Labels
Enhancement: General Server: PHP
Projects
None yet
Milestone
3.1.0
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@ybelenko @wing328 @ackintosh