Fix track2 ga naming and client merge issues #1250

qiaozha · 2021-11-18T05:07:10Z

No description provided.

qiaozha · 2021-11-19T08:21:13Z

test/integration/generated/datalakestorage/src/models/index.ts

@@ -180,7 +180,7 @@ export interface PathUpdateHeaders {
  /** User-defined properties associated with the file or directory, in the format of a comma-separated list of name and value pairs "n1=v1, n2=v2, ...", where each value is a base64 encoded string. Note that the string may only contain ASCII characters in the ISO-8859-1 character set. */
  properties?: string;
  /** When performing setAccessControlRecursive on a directory, the number of paths that are processed with each invocation is limited.  If the number of paths to be processed exceeds this limit, a continuation token is returned in this response header.  When a continuation token is returned in the response, it must be specified in a subsequent invocation of the setAccessControlRecursive operation to continue the setAccessControlRecursive operation on the directory. */
-  xMsContinuation?: string;


This is because Style.camel("XMsContinuation", true, {}, 5} gives us XMsContinuation

qiaozha · 2021-11-19T08:21:40Z

test/integration/generated/datalakestorage/src/models/index.ts

@@ -82,7 +82,7 @@ export interface BlobItemInternal {
  deleted: boolean;
  snapshot: string;
  versionId?: string;
-  isCurrentVersion?: boolean;


Not sure why we have this change

look like this is also because Style.camel("IsCurrentVersion", true, {}, 3) gives us IsCurrentVersion. Really weird

qiaozha · 2021-11-19T08:22:08Z

test/integration/generated/datalakestorage/src/models/index.ts

@@ -301,7 +301,7 @@ export interface PathGetPropertiesHeaders {
  /** The POSIX access permissions for the file owner, the file owning group, and others. Included in the response if Hierarchical Namespace is enabled for the account. */
  permissions?: string;
  /** The POSIX access control list for the file or directory.  Included in the response only if the action is "getAccessControl" and Hierarchical Namespace is enabled for the account. */
-  acl?: string;
+  ACL?: string;


probably should use override for this case?

qiaozha · 2021-11-19T08:27:07Z

test/integration/generated/lro/src/index.ts

@@ -7,6 +7,5 @@
 */

 export * from "./models";
-export { LROClient } from "./lROClient";
-export { LROClientContext } from "./lROClientContext";
+export { LROClient } from "./LROClient";


Does the file name change look ok ? This should also because of Style.camel("LRO") = "LRO"

The new name feels better!

qiaozha · 2021-11-19T08:28:36Z

test/integration/generated/lro/src/models/mappers.ts

@@ -198,10 +198,10 @@ export const SubProduct: coreClient.CompositeMapper = {
  }
 };

-export const LROsPatch200SucceededIgnoreHeadersHeaders: coreClient.CompositeMapper = {
+export const LROsPatch200SucceededIgnoreHeaders: coreClient.CompositeMapper = {


Not sure why we change it from HeadersHeaders to Headers ?

qiaozha · 2021-11-19T08:28:56Z

test/integration/generated/lro/src/operations/index.ts

 export * from "./lrosaDs";
-export * from "./lROsCustomHeader";
+export * from "./LROsCustomHeader";


The same as the previous one.

qiaozha · 2021-11-19T08:35:20Z

test/integration/generated/nameChecker/src/models/parameters.ts

-export const search$DONotNormalize$Text: OperationQueryParameter = {
-  parameterPath: "search$DONotNormalize$Text",
+export const searchDONOTNormalizeText: OperationQueryParameter = {
+  parameterPath: "searchDONOTNormalizeText",


Maybe this is because I set the Style.camel(str, true, {}, 5) but if I don't set it as 5, I get a mismatch in bodyComplex of CMYKColor in models/index.ts.

qiaozha · 2021-11-19T08:38:56Z

test/smoke/generated/network-resource-manager/src/models/index.ts

@@ -775,7 +775,7 @@ export interface BastionShareableLinkListRequest {
 /** Bastion Shareable Link. */
 export interface BastionShareableLink {
  /** Reference of the virtual machine resource. */
-  vm: Vm;
+  vm: VM;


Guess this is looking correct?

ramya-rao-a · 2021-11-22T16:42:16Z

@qiaozha The PR description is empty. Can you please update it with details of

links to issues you are resolving with this PR
your approach to resolving those issues
anything that the PR reviewer needs to know upfront before reviewing the PR

Both @sarangan12 and @joheredi are out this week. So, @deyaaeldeen will be reviewing this PR. Therefore, any context you can provide will be appreciated

ramya-rao-a · 2021-11-22T16:44:39Z

@deyaaeldeen I believe the issues related to this PR are #1131 and #1134

qiaozha · 2021-11-23T00:37:06Z

@deyaaeldeen This PR is trying to resolve issue #1131 #430 and #1134.
For issue #1131 and issue #430 @joheredi and I are trying to use Style.camel and Style.pascal from @azure-tools/codegen library to fix issue like method name JWTLogin will be convert to jWTLogin, but as I commentted above there's some weird behaviour and unexpected result of Style.camel. I am still trying to find out a solution, Could you provide any suggestions?
For issue #1134, we are trying to merge the Client and ClientContext into one file.

@ramya-rao-a @deyaaeldeen I will split them into two PRs seperately, as the fix for issue #1134 are pretty straight forward. But the naming issue fix probably going to take some time. I think that fix can be done after the GA, because it only impact some corner cases and we don't want this impact the GA date. let me know if that works for you ?

ramya-rao-a · 2021-11-23T05:07:24Z

Splitting the PR sounds like a good idea

qiaozha · 2021-11-30T00:44:10Z

test/integration/generated/nameChecker/src/operations/documents.ts

@@ -143,16 +143,16 @@ export class DocumentsImpl implements Documents {
   * Autocompletes incomplete query terms based on input text and matching terms in the index.
   * @param suggesterName The name of the suggester as specified in the suggesters collection that's part
   *                      of the index definition.
-   * @param search$DONotNormalize$Text The incomplete term which should be auto-completed.
+   * @param searchDONOTNormalizeText The incomplete term which should be auto-completed.


why there's a $ sign here before?

qiaozha · 2021-11-30T00:44:35Z

test/integration/generated/url2/src/operations/GET.ts

@@ -6,18 +6,18 @@
 * Changes may cause incorrect behavior and will be lost if the code is regenerated.


Does this file name look okay ?

qiaozha · 2021-11-30T00:45:23Z

test/integration/generated/xmlservice/src/models/index.ts

@@ -19,7 +19,7 @@ export interface RootWithRefAndNoMeta {
 /** I am a complex type with no XML node */
 export interface ComplexTypeNoMeta {
  /** The id of the res */
-  id?: string;
+  ID?: string;


Also this one.

qiaozha · 2021-11-30T00:52:08Z

test/integration/generated/datalakestorage/src/models/index.ts

@@ -119,7 +119,7 @@ export interface BlobPropertiesInternal {
  accessTierChangeTime?: Date;
  tagCount?: number;
  expiresOn?: Date;
-  isSealed?: boolean;
+  IsSealed?: boolean;


Is this 'Is' the problem ?

qiaozha · 2021-11-30T01:00:35Z

test/smoke/generated/network-resource-manager/src/operations/virtualWans.ts

@@ -163,13 +163,13 @@ export class VirtualWansImpl implements VirtualWans {
   * Creates a VirtualWAN resource if it doesn't exist else updates the existing VirtualWAN.
   * @param resourceGroupName The resource group name of the VirtualWan.
   * @param virtualWANName The name of the VirtualWAN being created or updated.
-   * @param wANParameters Parameters supplied to create or update VirtualWAN.
+   * @param WANParameters Parameters supplied to create or update VirtualWAN.


This is also a good candidate

src/utils/nameUtils.ts

sarangan12 · 2021-12-09T19:05:14Z

@qiaozha I have discussed with Jose about this PR and here is the summary:

Currently, the naming changes are done by 2 systems - M4 & SDK Generator.
Similarly, We could override the names using 2 methods - Swagger Overrides & M4 Overrides
After the discussion, we have come to the agreement that there can always be cases which might not be handled only by the M4 Logic and M4 Overrides. So, it is not safe or in our advantage to remove the naming logic we have in our SDK Generator.
The edge cases where the name is different now (from what we want) are rare and could be easily handled with a combination of Swagger Overrides & M4 Overrides.
So, I would recommend not to proceed with this PR and close it. The only recommended change would be to remove $DO_NOT_NORMALIZE$ and instead could use the modeler four overrides directly (as they could be read from the host). But, again it is not a mandatory or urgent change or a change that will produce any change in the generated code.

qiaozha · 2022-01-12T00:10:15Z

close this PR based on @sarangan12 's comment. Thanks

joheredi and others added 2 commits November 18, 2021 09:55

Naming and Context client merging

3994929

resolve name and merge clientcontext

5993e95

qiaozha requested review from joheredi, JonathanCrd and sarangan12 as code owners November 18, 2021 05:07

qiaozha added 7 commits November 18, 2021 13:55

fix-test

2f7f6d9

fix test

1d5a0b4

fix test

87e1be4

override ID

44bbd98

fix test

73a5c11

resolve test

e598dda

add mapper transform test back

26006b8

qiaozha commented Nov 19, 2021

View reviewed changes

qiaozha added 3 commits November 19, 2021 18:01

case sensitive

4cd8541

case sensitive

afb2d4d

file name case sensitive

d3f7694

qiaozha added 5 commits November 25, 2021 12:20

merge main and resolve conflicts

56803fc

fix test

097e64b

show git diff

133d7ab

try resolve ci failure

dcc9095

update test

6984eed

qiaozha commented Nov 30, 2021

View reviewed changes

deyaaeldeen reviewed Nov 30, 2021

View reviewed changes

src/utils/nameUtils.ts Outdated Show resolved Hide resolved

resolve comments

4d73305

qiaozha closed this Jan 12, 2022

qiaozha mentioned this pull request Feb 14, 2022

Fix sample generation for several mgmt plane RPs #1289

Merged

sarangan12 mentioned this pull request Jun 21, 2022

Why are Client Model Class Property names created with the first character lower case? #430

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix track2 ga naming and client merge issues #1250

Fix track2 ga naming and client merge issues #1250

qiaozha commented Nov 18, 2021

qiaozha Nov 19, 2021

qiaozha Nov 19, 2021

qiaozha Nov 19, 2021

qiaozha Nov 19, 2021

qiaozha Nov 19, 2021

deyaaeldeen Nov 30, 2021

qiaozha Nov 19, 2021

qiaozha Nov 19, 2021

qiaozha Nov 19, 2021

qiaozha Nov 19, 2021

ramya-rao-a commented Nov 22, 2021

ramya-rao-a commented Nov 22, 2021

qiaozha commented Nov 23, 2021

ramya-rao-a commented Nov 23, 2021

qiaozha Nov 30, 2021

qiaozha Nov 30, 2021

qiaozha Nov 30, 2021

qiaozha Nov 30, 2021

qiaozha Nov 30, 2021

sarangan12 commented Dec 9, 2021

qiaozha commented Jan 12, 2022

		@@ -6,18 +6,18 @@
		* Changes may cause incorrect behavior and will be lost if the code is regenerated.

Fix track2 ga naming and client merge issues #1250

Fix track2 ga naming and client merge issues #1250

Conversation

qiaozha commented Nov 18, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ramya-rao-a commented Nov 22, 2021

ramya-rao-a commented Nov 22, 2021

qiaozha commented Nov 23, 2021

ramya-rao-a commented Nov 23, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sarangan12 commented Dec 9, 2021

qiaozha commented Jan 12, 2022