Skip to content

[core-client] Bugfixes for unblocking data-tables#12547

Merged
xirzec merged 5 commits intoAzure:masterfrom
xirzec:coreClientFixes
Nov 17, 2020
Merged

[core-client] Bugfixes for unblocking data-tables#12547
xirzec merged 5 commits intoAzure:masterfrom
xirzec:coreClientFixes

Conversation

@xirzec
Copy link
Copy Markdown
Member

@xirzec xirzec commented Nov 13, 2020

This PR addresses a few things:

  • Cleans up pipeline creation for users of core-client.
  • Fixes substitution in paths when building operation URLs.
  • Avoids a problem where query params were getting unencoded during operation URL construction.

With these changes, I was able to get data-tables to work as expected and pass all integration tests.

@xirzec xirzec added Client This issue points to a problem in the data-plane of the library. Azure.Core labels Nov 13, 2020
@xirzec xirzec self-assigned this Nov 13, 2020
Copy link
Copy Markdown
Member

@deyaaeldeen deyaaeldeen left a comment

Choose a reason for hiding this comment

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

I think it looks good based on my limited experience. I left a few minor comments.

Copy link
Copy Markdown
Member

@joheredi joheredi left a comment

Choose a reason for hiding this comment

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

Looks awesome! Nice test coverage 😄

const pairs = queryString.split("&");

return pairs.map((pair) => {
const [name, value] = pair.split("=", 2);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

nit: this should be equivalent to

return pairs.map((pair) => pari.split("=", 2))

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I did it this way originally, but TS isn't smart enough to know the second parameter limits the size of the result and makes it into a 2 tuple, so it complains:

Type 'string[][]' is not assignable to type '[string, string][]'.
Type 'string[]' is missing the following properties from type '[string, string]': 0, 1

@xirzec
Copy link
Copy Markdown
Member Author

xirzec commented Nov 17, 2020

@jeremymeng would you like more time to review this PR or can I merge? 😃

Copy link
Copy Markdown
Member

@jeremymeng jeremymeng left a comment

Choose a reason for hiding this comment

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

LGTM

@xirzec xirzec merged commit 58a8ebd into Azure:master Nov 17, 2020
@xirzec xirzec deleted the coreClientFixes branch November 17, 2020 23:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Azure.Core Client This issue points to a problem in the data-plane of the library.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants