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

[WIP] Add PHP client #14

Merged
merged 5 commits into from
Mar 21, 2018

Conversation

ackintosh
Copy link
Contributor

@ackintosh ackintosh commented Feb 18, 2018

With reference to java, work in progress now. 👀

TODO

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

@jmini
Copy link
Contributor

jmini commented Feb 23, 2018

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
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
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
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
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)) {
Copy link
Contributor

@jmini jmini Feb 23, 2018

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
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
Copy link
Contributor

jmini commented Feb 23, 2018

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

@jmini
Copy link
Contributor

jmini commented Feb 24, 2018

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

@HugoMario
Copy link
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
Contributor Author

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

@jmini
Copy link
Contributor

jmini commented Mar 19, 2018

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
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
Copy link
Contributor

jmini commented Mar 21, 2018

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
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.

None yet

3 participants