search

eclipse-vertx / vertx-codegen

Vert.x code generator for asynchronous polyglot APIs
Apache License 2.0
105 stars 92 forks source link

A fix for the issue #295 #300

Closed cvgaviao closed 3 years ago

cvgaviao commented 4 years ago

Signed-off-by: Cristiano V. Gavião cvgaviao@gmail.com

A fix for #295

cvgaviao commented 4 years ago

@vietj, may I ask you to review this PR?

vietj commented 4 years ago

did you check this works with generators vertx-rx or service-proxy ?

cvgaviao commented 4 years ago

did you check this works with generators vertx-rx or service-proxy ?

Well, we are not using vertx-rx, but we are using service-proxy and vertx-web-api-service intensively in our project. Are there tests specifically for them?

vietj commented 4 years ago

did you try if it works with service-proxy ?

cvgaviao commented 4 years ago

did you try if it works with service-proxy ?

Well, with DataObject it is 100%, but with service-proxy not entirely. Only when returning the enum. If I use the enum as parameter, it is not using the Converter :( I've created this:

    Future<String> testEnum(RunMode mode);
    Future<RunMode> testEnum2(String mode);

and the resulted methods are:

        case "testEnum2": {
          service.testEnum2((java.lang.String)json.getValue("mode")).onComplete(res -> {
                      if (res.failed()) {
                        HelperUtils.manageFailure(msg, res.cause(), includeDebugInfo);
                      } else {
                        msg.reply(res.result() != null ? br.com.c8tech.mmarket.backend.models.JsonConverters.serializeRunMode(res.result()) : null);
                      }
                   });
          break;

    case "testEnum": {
      service.testEnum(json.getString("mode") == null ? null : br.com.c8tech.mmarket.backend.models.RunMode.valueOf(json.getString("mode"))).onComplete(HelperUtils.createHandler(msg, includeDebugInfo));
      break;
    }
cvgaviao commented 4 years ago

@vietj, do you have any clue about what I've missed ?

vietj commented 4 years ago

I don't know, that need to be studied

cvgaviao commented 4 years ago

I don't know, that need to be studied

I'm thinking that the problem is in the io.vertx.serviceproxy.generator.ServiceProxyGen class. More specifically in the generateAddToJsonStmt(ParamInfo, CodeWriter), sendTypeParameter() and wrapResult methods. They don't check about converters there.

cvgaviao commented 4 years ago

I don't know, that need to be studied

The issue that I found is definitively on io.vertx.serviceproxy.generator.ServiceProxyHandlerGen.generateJsonParamExtract(ParamInfo)

vietj commented 4 years ago

can you check also vertx-rx ? I think the same would apply

cvgaviao commented 4 years ago

can you check also vertx-rx ? I think the same would apply

Well, I don't use and don't know about this module. But I cloned and searched for enum references. I just found references on some TCK related classes...

Btw, how are those tck classes supposed to work?

vietj commented 4 years ago

TCK classes are created in this project and then generated in other projects with some tests to check it works according to expectations. Indeed we need something in TCK to test your new behavior.

cvgaviao commented 4 years ago

TCK classes are created in this project and then generated in other projects with some tests to check it works according to expectations. Indeed we need something in TCK to test your new behavior.

I think I did what was in my possibilities already...

cvgaviao commented 4 years ago

Hey @vietj, is there anything more I can do for you in this PR? I think I've already created the TCK tests that you referred and I've got no errors while building RX.

vietj commented 4 years ago

@cvgaviao we need implementations for vertx-rx, vertx-lang-kotlin-coroutines and vertx-service-proxy

cvgaviao commented 4 years ago

well, I already provided a PR for vertx-service-proxy.  I have not seen any need on vertx-rx, besides the tck that I've already did. :-(

wouldn't you take care of the vertx-lang-kotlin-coroutines in another time?

On 29/06/2020 04:36, Julien Viet wrote:

@cvgaviao https://github.com/cvgaviao we need implementations for vertx-rx, vertx-lang-kotlin-coroutines and vertx-service-proxy

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/vert-x3/vertx-codegen/pull/300#issuecomment-650987299, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAB5OVW5J4YNCLS36EBRRDDRZBAAJANCNFSM4M33DXOA.

cvgaviao commented 4 years ago

Hi @vietj. You didn't give me any feedback about all those related PRs. So, are you going to accept them? Because if you won't, I must research another way to deal with my customer's data.

vietj commented 3 years ago

@cvgaviao can you provide a new PR with a single commit and sign it along with the Eclipse Contributor Agreement ?

vietj commented 3 years ago

I would like to get this handled soon.

cvgaviao commented 3 years ago

@cvgaviao can you provide a new PR with a single commit and sign it along with the Eclipse Contributor Agreement ?

@vietj I have no free time to look at this in the next weeks.

vietj commented 3 years ago

@cvgaviao I just need you to merge all the existing commits in a single commit and sign the Eclipse Contributor Agreement and sign the commit , no code change is required

cvgaviao commented 3 years ago

@cvgaviao I just need you to merge all the existing commits in a single commit and sign the Eclipse Contributor Agreement and sign the commit , no code change is required

@vietj, I did the rebase. Let me know if you need anything else. thanks.

cvgaviao commented 3 years ago

I'm closing this forgotten branch and I going to add a new one.