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.

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

Feature/swagger parser upgrade #6338

Merged

Conversation

@chidozieononiwu chidozieononiwu requested a review from a team as a code owner June 13, 2023 21:05
@chidozieononiwu chidozieononiwu force-pushed the feature/SwaggerParserUpgrade branch from 9feb94d to 0223a23 Compare June 14, 2023 19:50
@heaths
Copy link
Member

heaths commented Jun 14, 2023

I'm afraid I don't have a lot of familiarity with this to really approve, though I'm happy to take a look conceptually.

With some of the changes I see, though, I want to make sure: are properties of models still listed breadth-first - which I fixed in #5802 - instead of depth-first like they were?

@chidozieononiwu
Copy link
Member Author

I'm afraid I don't have a lot of familiarity with this to really approve, though I'm happy to take a look conceptually.

With some of the changes I see, though, I want to make sure: are properties of models still listed breadth-first - which I fixed in #5802 - instead of depth-first like they were?

Based on request in #5582 I removed the nesting of models, instead I add each model to the definition where the properties are listed for each model.

@heaths
Copy link
Member

heaths commented Jun 15, 2023

I'm afraid I don't have a lot of familiarity with this to really approve, though I'm happy to take a look conceptually.
With some of the changes I see, though, I want to make sure: are properties of models still listed breadth-first - which I fixed in #5802 - instead of depth-first like they were?

Based on request in #5582 I removed the nesting of models, instead I add each model to the definition where the properties are listed for each model.

Is there a preview?

@chidozieononiwu chidozieononiwu force-pushed the feature/SwaggerParserUpgrade branch 3 times, most recently from d931b7b to 35d1e03 Compare June 17, 2023 02:22
@maririos
Copy link
Member

Is there a preview?

@heaths description was updated to show the differences

@maririos
Copy link
Member

@mikekistler coudl you also take a look at this PR?

@maririos maririos requested a review from mikekistler June 20, 2023 16:19
@mikekistler
Copy link
Member

I thought the plan for fixing the confusing flattening of models #5582 was to link to the model definition rather than expanding it in the request or response body section. But the the preview above there are no links:

image

@mikekistler
Copy link
Member

@chidozieononiwu Could you post a list of the improvements here -- a brief summary is fine, something like what we would post in the change log and advertise to the team.

@chidozieononiwu
Copy link
Member Author

I thought the plan for fixing the confusing flattening of models #5582 was to link to the model definition rather than expanding it in the request or response body section. But the the preview above there are no links:

image

You are right. The actual linking will need to be a UI change yet to be made.

Copy link
Member

@praveenkuttappan praveenkuttappan 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.

@chidozieononiwu chidozieononiwu force-pushed the feature/SwaggerParserUpgrade branch from 35d1e03 to 7d5fdde Compare June 26, 2023 20:23
@chidozieononiwu chidozieononiwu merged commit 5aeaa35 into Azure:main Jun 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

5 participants