Azure Core VNext and ClientCore updates#7115
Conversation
srnagar requested review from anuchandy, weidongxu-microsoft, haolingdong-msft and XiaofeiCao as code owners
microsoft-github-policy-servicebot added the emitter:client:java 
No changes needing a change description found. |
You can try these changes here
|
srnagar requested review from bterlson, markcowl, allenjzhang, timotheeguerin, witemple-msft, chrisradek and AlitzelMendez as code owners...in/java/com/microsoft/typespec/http/client/generator/core/extension/plugin/JavaSettings.javaShow resolvedHide resolved
...in/java/com/microsoft/typespec/http/client/generator/core/extension/plugin/JavaSettings.javaShow resolvedHide resolved
...main/java/com/microsoft/typespec/http/client/generator/core/model/clientmodel/ClassType.javaShow resolvedHide resolved
...src/main/java/com/microsoft/typespec/http/client/generator/core/model/clientmodel/Proxy.javaShow resolvedHide resolved
| @@ -413,6 +414,44 @@ private void addPagingNextOperation(Client client, OperationGroup operationGroup | |||
| nextOperation.setSummary(operation.getSummary()); | |||
| nextOperation.setUid(operation.getUid()); | |||
| if (operation.getConvenienceApi() != null && operation.getConvenienceApi().getRequests() != null) { | |||
There was a problem hiding this comment.
Is there major design change on nextLink call in v2?
| if (JavaSettings.getInstance().isBranded()) { | ||
| builder.convenienceMethods(convenienceMethods); | ||
| } |
There was a problem hiding this comment.
Why this is turned off in both unbranded and v2?
nit, if need to turn off, the condition better to include the final List<ConvenienceMethod> convenienceMethods code as well.
Also, the convenience method is used to handle the method signature override in TypeSpec, example here https://github.com/microsoft/typespec/blob/main/packages/http-client-java/generator/http-client-generator-test/tsp/method-override.tsp
Unsure whether this would be affected.
| private static final ObjectMapper OBJECT_MAPPER | ||
| = new ObjectMapper().setSerializationInclusion(JsonInclude.Include.NON_NULL); |
There was a problem hiding this comment.
nit, a bit weird that we are still using Jackson in our code.
...src/main/java/com/microsoft/typespec/http/client/generator/core/template/TemplateHelper.javaShow resolvedHide resolved
...in/java/com/microsoft/typespec/http/client/generator/core/model/javamodel/JavaInterface.javaShow resolvedHide resolved
.../src/main/java/com/microsoft/typespec/http/client/generator/core/template/ProxyTemplate.javaShow resolvedHide resolved
| @@ -121,6 +135,7 @@ public ConfidentialResourceInner withLocation(String location) { | |||
| /** | |||
| * {@inheritDoc} | |||
| */ | |||
| @Generated | |||
There was a problem hiding this comment.
The @Generated annotation is written in mgmt SDK, but it is not consistent. E.g. validate, toJson and fromJson does not have it.
Can we keep it not generated, till we can make them consistent?
| BinaryData request = BinaryData.fromObject(node); | ||
| client.putWithResponse(request, null); | ||
| client.putWithResponse(model.map, null); |
There was a problem hiding this comment.
Many of the changes in clientcore Tests files about request body seems to be bug from codegen (or major changes in design of the client method?)?
| private final Map<CacheKey, List<ClientMethod>> parsed = new ConcurrentHashMap<>(); | ||
| private static class CacheKey { |
There was a problem hiding this comment.
I don't think we need to duplicate these.
Also, this file seems pretty big, and potentially have lots of duplication with ClientMethodMapper. @anuchandy
| import java.util.function.Function; | ||
| import java.util.stream.Collectors; | ||
| public class ClientCoreClientMethodTemplate extends ClientMethodTemplate { |
There was a problem hiding this comment.
This is also a big file, and potentially have duplication.
Can we at least flag what we've changed in this subclass, and then at least have a clue about what to refactor when later trying to reduce the duplications?
There was a problem hiding this comment.
Unblock.
There are a few things I have concern
- clientcore tests are disabled. It seems either the codegen or annotation processor have major problem (otherwise I'd expect just disabling a few test cases)
- There be 1.0.0 compiler release at about May 4, we would likely need to release a working unbranded emitter on this version
- We can merge this, and keep release unbranded emitter from feature branch on clientcore beta.5 for a few releases. But we'd expect the new clientcore work before long, as such release from branch would have large Eng cost (and likely would eventually fail when it diffs too much from main)
- Code around ClientMethod(Mapper/Template) have potentially large duplications. I'd hope we know what's the diff between the baseclass and subclass, so that is be possible to reduce the duplication in future.
- Turn of
convenienceAPIwould be a concern Azure Core VNext and ClientCore updates #7115 (comment) - We want to avoid generating
@Generatedin mgmt Azure Core VNext and ClientCore updates #7115 (comment) (for now)
| @@ -266,6 +269,7 @@ words: | |||
| - xiangyan | |||
| - xiaofei | |||
| - xlarge | |||
| - Xlint | |||
There was a problem hiding this comment.
Is there anything about Xlint?
No description provided.