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

Fixes OpenAPI Links and makes them optional #72

Merged
merged 20 commits into from Jul 20, 2020

Conversation

@irvinesunday
Copy link
Contributor

@irvinesunday irvinesunday commented Jul 7, 2020

Ths PR fixes #64

Proposes:

  • Fixing the operationIds and parameters of generated OpenAPI Links.
  • Making optional the generation of OpenAPI Links through a boolean setting.
  • Updating the Multiple.Schema.OpenApi.json and Multiple.Schema.OpenApi.yaml test docs. with sample OpenAPI Link tags and content and updating their respective tests appropriately by enabling Links generation.
  • Removing the OpenAPI Links tags from the other test docs.
@@ -44,15 +44,15 @@ public virtual OpenApiOperation CreateOperation(ODataContext context, ODataPath
// Security
SetSecurity(operation);

// Parameters

This comment has been minimized.

@xuzhg

xuzhg Jul 7, 2020
Collaborator

The place of function matters here? #Resolved

This comment has been minimized.

@irvinesunday

irvinesunday Jul 7, 2020
Author Contributor

Yes. It needs to be processed before SetResponses() so that we can use the parameters in the argument here when calling the CreateLinks() method; which will get used here. #Resolved

IDictionary<string, OpenApiLink> links = new Dictionary<string, OpenApiLink>();
foreach (IEdmNavigationProperty np in entityType.DeclaredNavigationProperties())

if (!targetMultiplicity)

This comment has been minimized.

@xuzhg

xuzhg Jul 7, 2020
Collaborator

if (!targetMultiplicity) [](start = 12, length = 24)

if targetMultiplicity == true, this function seems doing nothing?
Why don't you check this boolean out of this function call?

If (!targetMultiplicity)
{
var links = context.CreateLinks(...);
} #Resolved

This comment has been minimized.

@irvinesunday

irvinesunday Jul 7, 2020
Author Contributor

Good catch. Resolved. #Resolved

Copy link
Collaborator

@xuzhg xuzhg left a comment

🕐

@@ -44,15 +44,15 @@ public virtual OpenApiOperation CreateOperation(ODataContext context, ODataPath
// Security
SetSecurity(operation);

// Parameters

This comment has been minimized.

@xuzhg

xuzhg Jul 15, 2020
Collaborator

// Parameters [](start = 12, length = 13)

Please add a detail comments here to maintenance. For example, we must call SetParameters ahead of ...., and Why?

This comment has been minimized.

@irvinesunday

irvinesunday Jul 20, 2020
Author Contributor

Added helpful comment.
Resolved

@xuzhg
xuzhg approved these changes Jul 15, 2020
Copy link
Collaborator

@xuzhg xuzhg left a comment

:shipit:

Irvine Sunday added 2 commits Jul 17, 2020
Irvine Sunday
Irvine Sunday
@darrelmiller darrelmiller merged commit 9d44019 into microsoft:master Jul 20, 2020
2 checks passed
2 checks passed
OpenAPI.OData-Master-Rolling Build #OData_OpenAPI.OData-Master-Rolling_merge_20200717.2 succeeded
Details
license/cla All CLA requirements met.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

3 participants
You can’t perform that action at this time.