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

Fix petstore #191

Merged
merged 16 commits into from
Jun 1, 2021
Merged

Fix petstore #191

merged 16 commits into from
Jun 1, 2021

Conversation

darrelmiller
Copy link
Member

This is a partial attempt to fix the returning of the pets collection in petstore. It is not a complete solution.

} else if (schema.IsArray())
{
// collection of referenced schema
var type = CreateModelClassAndType(rootNode, currentNode, schema, operation, parentElement, codeNamespace, "");
Copy link
Member Author

Choose a reason for hiding this comment

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

Should this have the suffix "Collection" and not be set of CollectionKind Array, or should it by type Pet[] ?

Copy link
Member

Choose a reason for hiding this comment

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

we should go with an array of pets, I'd like to avoid generating extra classes when it's not needed. We might have to tweak the deserialization process and/or the executor body during testing but this scenario should mostly already be handled. (besides the generation part)

} else if (schema.IsArray())
{
// collection of referenced schema
var type = CreateModelClassAndType(rootNode, currentNode, schema, operation, parentElement, codeNamespace, "");
Copy link
Member

Choose a reason for hiding this comment

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

we should go with an array of pets, I'd like to avoid generating extra classes when it's not needed. We might have to tweak the deserialization process and/or the executor body during testing but this scenario should mostly already be handled. (besides the generation part)

@baywet
Copy link
Member

baywet commented May 31, 2021

@PureKrome I know our discussions were a bit scattered on the pet store subject, sorry about that, let's regroup here if you don't mind :)

@baywet baywet added this to the Alpha milestone May 31, 2021
@baywet baywet self-assigned this May 31, 2021
@PureKrome
Copy link
Contributor

@PureKrome I know our discussions were a bit scattered on the pet store subject, sorry about that, let's regroup here if you don't mind :)

baywet
baywet previously approved these changes May 31, 2021
@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

80.4% 80.4% Coverage
0.0% 0.0% Duplication

@baywet baywet marked this pull request as ready for review May 31, 2021 20:10
@baywet
Copy link
Member

baywet commented May 31, 2021

@PureKrome: just to recap offline conversations. With this PR you'll be able to generate petstore. However the generated result probably won't build or execute some of the requests properly. That's because we need to relax some type contraint and consolidate the httpcore interface. This additional work is captures in #193 and will be done in another PR. Thanks for all the feedback again!

@PureKrome
Copy link
Contributor

Really looking forward to playing around with an generated "Petshop" client! Thanks heaps for putting up with my nagging on this - it's a big stepping stone for me (and IMO, other people exploring this project)

@darrelmiller
Copy link
Member Author

:shipit:

@baywet baywet merged commit 389e4e2 into main Jun 1, 2021
@baywet baywet deleted the fix-petstore branch June 1, 2021 13:59
@PureKrome
Copy link
Contributor

Oh nice! very exciting! I just used VS to generate a client (after pulling down main) and a client was generated!

But .. i still need the Kiota.* nugets from the GH feed .. which means I need to setup a PAT (i was trying to avoid this -the entire time-) .. so i'll go to 💤 now and try it tomorrow'ish :)

exciting!

@baywet
Copy link
Member

baywet commented Jun 1, 2021

yep, this is something we plan to get rid of with #122 and #128. But we don't want to publish packages if we're changing them a lot to avoid breaking people. #126 and #193 are what we believe to be the last breaking changes that are planned at this time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants