Modify, clean, refactor for the OpenApiDocument and OpenApiPaths by xuzhg · Pull Request #46 · microsoft/OpenAPI.NET

xuzhg · 2017-10-31T04:49:24Z

Modify, cleanup, refactor, correct for OpenApiDocument
Create a new class named OpenApiDictionaryElement
Modify, cleanup, refactor, correct for OpenApiPaths

PerthCharern · 2017-10-31T18:29:57Z

Let's document public properties.

Refers to: src/Microsoft.OpenApi/Models/OpenApiDictionaryElement.cs:91 in e6a33b8. [](commit_id = e6a33b8, deletion_comment = False)

PerthCharern · 2017-10-31T18:30:41Z

-            writer.WriteObject("info", Info, (w, i) => i.WriteAsV3(w));
-            writer.WriteList("servers", Servers, (w, s) => s.WriteAsV3(w));
-            writer.WritePropertyName("paths");
+            // openapi


// openapi [](start = 12, length = 10)

These comments look cluttered to me. I don't think they are necessary. What do you think?

I suggest to leave these comments there for better understanding which parts are being written.
Is it ok for you?

PerthCharern · 2017-10-31T18:31:46Z

-            writer.WriteList("servers", Servers, (w, s) => s.WriteAsV3(w));
-            writer.WritePropertyName("paths");
+            // openapi
+            writer.WriteStringProperty(OpenApiConstants.OpenApiDocOpenApi, "3.0.0");


"3.0.0" [](start = 75, length = 7)

Leverage your constant

Or use the Version property in this class

PerthCharern · 2017-10-31T18:35:07Z

+        /// <summary>
+        /// REQUIRED.This string MUST be the semantic version number of the OpenAPI Specification version that the OpenAPI document uses.
+        /// </summary>
+        public Version Version { get; set; } = OpenApiConstants.OpenApiDocDefaultVersion;


Does this need to exist here? It's not in the spec. #Closed

Oh. I see what's being done here. This corresponds to the "openapi" property in the spec. Can we name this SpecificationVersion to avoid confusion with the other version in the Info?

PerthCharern · 2017-10-31T18:37:15Z

+            writer.WriteStringProperty(OpenApiConstants.OpenApiDocSwagger, "2.0");
+
+            // info
            writer.WriteObject("info", Info, (w, i) => i.WriteAsV2(w));


"info" [](start = 31, length = 6)

leverage your constant

PerthCharern · 2017-10-31T18:42:55Z

        writer.WriteEndArray();

This logic should be in OpenApiServer.WriteAsV2

(The same principle should apply to the SecurityScheme/SecurityRequirement stuff as well)

Refers to: src/Microsoft.OpenApi/Models/OpenApiDocument.cs:189 in e6a33b8. [](commit_id = e6a33b8, deletion_comment = False)

PerthCharern · 2017-10-31T18:44:24Z

+{
+    internal static class OpenApiDocumentTestHelper
+    {
+        public static void CreatePath(this OpenApiDocument doc, string key, Action<OpenApiPathItem> configure)


public [](start = 8, length = 6)

Document. Also, where is this used?

It seems only used in the ExampleTests.cs and ValidationTests.cs. So, I move it from OpenApiDocument to test project.

changed "doc" to "document"

PerthCharern · 2017-10-31T18:45:48Z

+            writer.WriteExtensions(Extensions);
+
+            writer.WriteEndObject();
        }


I'd rather have WriteAsV2 and WriteAsV3 implement this logic separately. It might be duplicate code, but technically they are somewhat independent, and it will be easier to fix them individually if needs be in the future.

What do you think?

Either is ok for me. But I changed follow up your suggestion.

PerthCharern · 2017-10-31T18:47:00Z

Let's add more extensive unit tests for paths.

xuzhg · 2017-10-31T21:32:24Z

1. Let's add more extensive unit tests for paths.

I want to leave it when I back to change "OpenApiPathItem".

This logic should be in OpenApiServer.WriteAsV2 (The same principle should apply to the SecurityScheme/SecurityRequirement stuff as well)
I am not familiar with V3 -> V2. But, it supposed to leave it when I back to change "OpenApiSe...".

Let's document public properties.

done.

PerthCharern · 2017-10-31T22:24:57Z

-            writer.WriteObject("info", Info, (w, i) => i.WriteAsV2(w));
-            SerializeHostInfo(writer, Servers);
+            // swagger
+            writer.WriteStringProperty(OpenApiConstants.OpenApiDocSwagger, OpenApiConstants.OpenApiDocSwaggerVersion);


OpenApiConstants.OpenApiDocSwaggerVersion [](start = 75, length = 41)

If above uses SpecVersion, we should use SpecVersion here too.

PerthCharern · 2017-10-31T22:30:08Z

@xuzhg I can take up the V2 object writing issues. I'll sign off.

PerthCharern

xuzhg · 2017-10-31T22:36:42Z

Great, Thanks.

docs: adds the v3 upgrade guide

PerthCharern reviewed Oct 31, 2017

View reviewed changes

xuzhg added 3 commits October 31, 2017 15:28

Modify, clean, refactor for the OpenApiDocument and OpenApiPaths

a202ff0

resolve the comments for the OpenApiDocument related

62bafd9

change SerializeHostInfo to WriteHostInfo

dd2a9db

PerthCharern approved these changes Oct 31, 2017

View reviewed changes

xuzhg merged commit 9df3f3d into microsoft:master Oct 31, 2017

xuzhg deleted the DocumentAndPathsChanges branch October 31, 2017 22:36

baywet added a commit that referenced this pull request Nov 11, 2025

Merge pull request #46 from BinkyLabs/docs/upgrade-guide

e29b387

docs: adds the v3 upgrade guide

Uh oh!

Conversation

xuzhg commented Oct 31, 2017

Uh oh!

PerthCharern commented Oct 31, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PerthCharern Oct 31, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PerthCharern commented Oct 31, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PerthCharern commented Oct 31, 2017

Uh oh!

xuzhg commented Oct 31, 2017

Uh oh!

PerthCharern Oct 31, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PerthCharern commented Oct 31, 2017

Uh oh!

PerthCharern left a comment

Choose a reason for hiding this comment

Uh oh!

xuzhg commented Oct 31, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

PerthCharern Oct 31, 2017 •

edited

Loading

PerthCharern Oct 31, 2017 •

edited

Loading