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.

Sign up for GitHub

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

Jump to bottom

RestError to have details that point to swagger specified error shape #5437

Merged
merged 10 commits into from
Oct 19, 2019
Merged

RestError to have details that point to swagger specified error shape #5437

merged 10 commits into from
Oct 19, 2019

Conversation

sarangan12
Copy link
Member

This is related to #4999.

In order to resolve the issue, the changes have to happen in 3 Places

  1. Autorest.typescript SDK Generator to update the mappings (Completed)
  2. Changes in DeserializationPolicy in core-http. This will deserialize the headers in the error response.
  3. Changes in serviceClient in core-http. This will flatten the response so the user coulkd read it.

xirzec
xirzec reviewed Oct 8, 2019
View reviewed changes
Copy link
Member

@xirzec xirzec left a comment

Choose a reason for hiding this comment

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

Looks so good I could only find pedantic style things to comment on. 😉

sdk/core/core-http/lib/policies/deserializationPolicy.ts Outdated Show resolved Hide resolved
sdk/core/core-http/lib/restError.ts Outdated Show resolved Hide resolved
sdk/core/core-http/lib/restError.ts Outdated Show resolved Hide resolved
sdk/core/core-http/lib/restError.ts Outdated Show resolved Hide resolved
sdk/core/core-http/lib/restError.ts Outdated Show resolved Hide resolved
sdk/core/core-http/lib/serviceClient.ts Outdated Show resolved Hide resolved
sdk/core/core-http/test/policies/deserializationPolicyTests.ts Outdated Show resolved Hide resolved
daviwil
daviwil approved these changes Oct 8, 2019
View reviewed changes
Copy link
Contributor

@daviwil daviwil left a comment

Choose a reason for hiding this comment

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

Looks good, though I agree with many of Jeff's comments too

sdk/core/core-http/lib/restError.ts Outdated Show resolved Hide resolved
@sarangan12 sarangan12 changed the title Release default headers [DO NOT MERGE] Release default headers Oct 16, 2019
@ramya-rao-a
Copy link
Contributor

/azp run js - core - ci

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@sarangan12
Copy link
Member Author

/azp run js - core - ci

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@sarangan12
Copy link
Member Author

/azp run js - core - ci

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@sarangan12
Copy link
Member Author

/azp run js - core - ci

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@ramya-rao-a ramya-rao-a merged commit b13f10a into Azure:master Oct 19, 2019
@ramya-rao-a ramya-rao-a changed the title Release default headers RestError to have details that point to swagger specified error shape Oct 19, 2019
@sarangan12 sarangan12 mentioned this pull request Oct 19, 2019
Merged
@sarangan12 sarangan12 mentioned this pull request Apr 28, 2020
Closed
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@xirzec xirzec xirzec left review comments

@ramya-rao-a ramya-rao-a ramya-rao-a approved these changes

@daviwil daviwil daviwil approved these changes

@bterlson bterlson bterlson approved these changes

@jeremymeng jeremymeng Awaiting requested review from jeremymeng

@sophiajt sophiajt Awaiting requested review from sophiajt

@sadasant sadasant Awaiting requested review from sadasant

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@sarangan12 @ramya-rao-a @bterlson @daviwil @xirzec