Skip to content

[WIP] Add PHP client - #14

Merged
HugoMario merged 5 commits into
swagger-api:php_client_generatorfrom
ackintosh:php-client
Mar 21, 2018
Merged

HugoMario merged 5 commits into
swagger-api:php_client_generatorfrom
ackintosh:php-client

Conversation

@ackintosh

@ackintosh ackintosh commented Feb 18, 2018 •

Copy link
Copy Markdown
Contributor

With reference to java, work in progress now. 👀

TODO

  • Fix build error
  • Make PhpClientCodegen/AbstractPhpCodegen DRY
  • etc...

@webron
webron requested a review from HugoMario February 19, 2018 18:29
@jmini

jmini commented Feb 23, 2018

Copy link
Copy Markdown
Contributor

First idea: you should merge master to your feature branch or rebase all your commit on top of master.

This is what I did locally to review your code.

import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

public abstract class AbstractPhpCodegen extends DefaultCodegenConfig {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

DefaultCodegenConfig is not imported. add following line:

import io.swagger.codegen.languages.DefaultCodegenConfig;

import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

public class PhpClientCodegen extends DefaultCodegenConfig {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Same here: DefaultCodegenConfig is not imported.

@@ -0,0 +1,731 @@
package io.swagger.codegen.languages;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Package should be: io.swagger.codegen.languages.php

@@ -0,0 +1,667 @@
package io.swagger.codegen.languages;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Package should be: io.swagger.codegen.languages.php


if (example == null) {
example = "NULL";
} else if (Boolean.TRUE.equals(p.isListContainer)) {

@jmini jmini Feb 23, 2018 •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This produces a compile error by p.isListContainer.

Where did you get this file? AbstractPhpCodegen on the 3.0.0 branch in the swagger-codegen has no copile error. You should have took this as base.


if (example == null) {
example = "NULL";
} else if (Boolean.TRUE.equals(p.isListContainer)) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This produces a compile error by p.isListContainer.

Where did you get this file? PhpClientCodegen on the 3.0.0 branch in the swagger-codegen has no copile error. You should have took this as base.

@jmini

jmini commented Feb 23, 2018

Copy link
Copy Markdown
Contributor

I hope you will manage to remove the compile errors. Do not hesitate to ask if you need something.

@jmini

jmini commented Feb 24, 2018

Copy link
Copy Markdown
Contributor

In order to help you, I have started a wiki page:
Swagger Codegen migration (swagger codegen generators repository)

@HugoMario

Copy link
Copy Markdown
Contributor

@ackintosh do you mind if we merge your fork with a different branch, in order to help with TO-DO tasks? or can you please grant access to HugoMario user in your forked repo?

@ackintosh

Copy link
Copy Markdown
Contributor Author

Sorry for late reply 💦
No, I don't. Please start your work in a different branch. ✨

@jmini

jmini commented Mar 19, 2018

Copy link
Copy Markdown
Contributor

I have automated a lot of the work that is requested to migrate the generators, this is no problem for me to migrate the PHP generator.

But I will not be able to evaluate the generated code and to confirm that it works as expected.

How do you want to proceed? Should I start something and you will test it?
@HugoMario any other idea?

@HugoMario
HugoMario changed the base branch from master to php_client_generator March 20, 2018 21:32
@HugoMario

Copy link
Copy Markdown
Contributor

thanks @ackintosh
@jmini sure, let me merge this into a new branch php_client_generator, so you can add your work there and i can later verify the generated code.

@jmini

jmini commented Mar 21, 2018

Copy link
Copy Markdown
Contributor

I have fetched the php_client_generator branch, but there is nothing more than on the master branch. I think you should merge ackintosh/php-client on your new branch.

What is the idea?
Should I create a pull request for the new php_client_generator branch, should I push directly on the php_client_generator branch (but I think that I need a permission for that).

I can also fetch ackintosh/php-client locally, push the changes I had described in this merge request and then open a new one.


@HugoMario: I would appreciate if you could review and merge my existing pull requests for the swagger-codegen-generators and swagger-codegen repositories.

There is a lot important stuff going on in order to improve Swagger v3.

@HugoMario
HugoMario merged commit 25843ab into swagger-api:php_client_generator Mar 21, 2018
@HugoMario

Copy link
Copy Markdown
Contributor

@jmini i already merge this PR against php_client_generator branch. So the idea is that any change you want to make, you can do it pointing to this branch.

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.

3 participants