add the write functionality for each open api element by xuzhg · Pull Request #42 · microsoft/OpenAPI.NET

xuzhg · 2017-10-27T20:12:10Z

1 .Separate the functions into the Open Api element. See related PR #41

Change the OpenApiSerializer to OpenApiElementSerializeExtension.

xuzhg · 2017-10-27T20:14:36Z

@PerthCharern
Please take a look at #42. And Compare between #41 and #42.

xuzhg · 2017-10-27T21:11:53Z

@PerthCharern

Please take a look this PR.

I will work on the code improvement/cleanup for each OpenApiXXXX, and will add Unit test for each OpenApiXXXX.

PerthCharern · 2017-10-27T21:16:10Z

src/Microsoft.OpenApi/Models/OpenApiCallback.cs

virtual [](start = 15, length = 7)

No need for virtual ?

Maybe I am wrong.

I want to give customer a functionality to create their own OpenApiXXX and derived from "OpenApiCallback", they can override these methods.

Interesting - I didn't think of that scenario. I'm not sure how they would use that new class they created since the objects are defined here with classes we have. It would be a cool thing nonetheless to allow this to be customizable.

Do you happen to have some specific use case in mind?

So far no. But who knows. 😄

PerthCharern · 2017-10-27T21:18:43Z

I prefer #42 over #41. IMO, Extension methods are unnecessary in this scenario and lead to overabundance of classes.

PerthCharern · 2017-10-27T21:20:45Z

src/Microsoft.OpenApi/Models/OpenApiDocument.cs

+            writer.WriteList("servers", Servers, (w, s) => s.WriteAsV3(w));
+            writer.WritePropertyName("paths");
+
+            writer.WriteStartObject();


writer.WriteStartObject(); [](start = 11, length = 27)

This Start/End object should be moved to the Paths object to be consistent with the other models.

Yes. That's what I will work to change/refactor each "OpenApiXXX".

I don't understand the value of making this change. I only see disadvantages.

@darrelmiller Could you elaborate? That is how it's being done for other objects.

@xuzhg @darrelmiller I still think this one-off inconsistency should be fixed. But if you'd rather do this later, I can overlook this for now.

Actually, we probably have to do this the other way round since some two objects in the DOM can be one object in the document when we do V3 -> V2.

xuzhg · 2017-10-27T22:21:39Z

@PerthCharern

Please see my commit 3, I will do like this for all OpenApiXXX one by one.

darrelmiller · 2017-10-30T15:51:25Z

src/Microsoft.OpenApi/Models/OpenApiContact.cs

Why are we putting writer function back into the model classes? It was intentional not to do this. The idea was to have all structural rules relating to a particular version in a single class, not scattered across the model classes.

…odes

xuzhg · 2017-10-30T21:32:51Z

@darrelmiller @PerthCharern

So, we discussed to take this PR to move forward, would you please take a look and sign off?

For change to the "public virtual void WriteAsV2(3)" as "internal virtual void WriteAsV2(3)", I have to say that I need this to be public then I can use it in the generic serialize function for customer to use.

darrelmiller · 2017-10-30T21:45:23Z

@xuzhg I'm sure I follow why the WriteAs methods need to be public. The Writers/Serializers that need them are in the same project, no?

xuzhg · 2017-10-30T21:48:00Z

@darrelmiller

If I make them internal, I can't use the "interface", Then I can't use it as generic method in the following file:
https://github.com/Microsoft/OpenAPI.NET/pull/42/files#diff-214dd7887368cc85c3762d5b0ef3c289

darrelmiller · 2017-10-30T21:59:32Z

Can we then at least make the interface implementation explicit so that we don't clutter up the public interface of the DOM classes?

xuzhg · 2017-10-30T22:04:11Z

src/Microsoft.OpenApi/Writers/OpenApiYamlWriter.cs

                else
                {
-                    Writer.WriteLine();
+                    if (!IsTopLevelObjectScope())


@PerthCharern I think we should add this back because "an extra line break is not reasonable for the single property."

Not sure if I fully agree with that. I don't see the point of having this as a special case. Adding a line break in the beginning is not wrong, and makes more sense if we plan to start the doc with ---.

If we will not start the doc with ---, then I think it makes sense that you are adding it back.

@PerthCharern

Can I file an issue to improve the yaml writer later? To be honest, I am not familiar with the Yaml spec a lot. So, the Yaml writer maybe has some violation with the Spec.

xuzhg · 2017-10-30T22:04:54Z

test/Microsoft.OpenApi.Tests/Models/OpenApiContactTests.cs

@darrelmiller what do you think about the "empty" yaml. It's empty or a whitespace?

I would expect it to be empty. Non-significant whitespace does not tend to get output by YAML serializers. http://yaml.org/spec/1.2/spec.html#id2775170

xuzhg · 2017-10-30T22:24:24Z

@darrelmiller @PerthCharern

I make the method as internal in the latest commit. Would you please take a look?

PerthCharern · 2017-10-30T22:44:59Z

test/Microsoft.OpenApi.Tests/Writers/OpenApiYamlWriterTests.cs


-            expectedYaml = expectedYaml.MakeLineBreaksEnvironmentNeutral();
-
            // Assert


I think we should put back expectedYaml = expectedYaml.MakeLineBreaksEnvironmentNeutral();

This makes sure this test passes in any OS environment it's run.

Yes. I totally agree. But it is related to the discussion of "New line break at the starting of the Yaml document". I suggest to leave it to fix at #39. Is it ok?

This one is not really related to that though. This applies to all line breaks. #Closed

Ok. I re-added it back. #Closed

PerthCharern · 2017-10-30T22:46:40Z

src/Microsoft.OpenApi/OpenApiElementSerializeExtensions.cs

"Not supported Open Api document format!" [](start = 47, length = 41)

$"The given OpenAPI format is not supported: {format}."

Thanks, I changed it and move it to SRResource.resx.

PerthCharern · 2017-10-30T22:47:15Z

src/Microsoft.OpenApi/OpenApiElementSerializeExtensions.cs

"Unknown Open API specification version." [](start = 47, length = 41)

$"The given OpenAPI specification version is not supported: {specVersion}."

Thanks, I changed it and move it to SRResource.resx.

… internal

xuzhg · 2017-10-30T23:16:08Z

@darrelmiller @PerthCharern

Sorry, I think That's all commits for this PRs. I continue on the clean/refactor for other DOM and try to add Unit test for each of them. I will send another PRs for them.

PerthCharern · 2017-10-30T23:31:37Z

src/Microsoft.OpenApi/Models/OpenApiPaths.cs

changed. #Closed

PerthCharern · 2017-10-30T23:34:15Z

src/Microsoft.OpenApi/Models/OpenApiSecurityRequirement.cs

+                }
+                else
+                {
+                    writer.WriteValue("[]");


I'd say some hardcoded value like this should be the responsibility of the writer. The DOM should call something like writer.WriteEmptyArray()

Agreed. I am working on cleanup/refactor the DOM one by one.

PerthCharern · 2017-10-30T23:37:21Z

src/Microsoft.OpenApi/Models/OpenApiOAuthFlow.cs

This is going to be an issue since for V2, these properties are a part of the SecurityScheme object, not a new object.

Yes. Please see the WriteAsV2(writer) in the OpenApiSecurityScheme.cs

Right. I saw that. That's why this will be an issue. We should not be writing the start of a new object. These properties here are a part of the Scheme object.

It might make more sense to put all these WriteStartObject and WriteEndObject in the caller, so that you can choose to call or not call whenever you want. If we go that route, we should consistently go that route for all calls.

Oh, yes. let me think it.

I'm fine with checking this overall architecture in first. We can tackle these isolated issues separately if you like.

Originally, I want to take this DOM cleanup/refactor later. But for this you mentioned, I changed it in my latest commit. Would you please take a look?

PerthCharern · 2017-10-30T23:38:55Z

test/Microsoft.OpenApi.Tests/StringExtensions.cs

Please don't do this in this extension. If you need this functionality, then please separate it to a new extension.

Ok. I changed it.

PerthCharern

It's great that we can unit test each element now. Thanks for making this change Sam.

There are still a couple of open issues, but they are isolated, so I'll sign off, so that we can get this overall structure checked in and start working on those bugs.

Where should WriteStartObject / WriteEndObject go?
Some DOMs might not be 100% correct yet (e.g. SecurityScheme). We need to fix them.
Newline issues in YAML/Extensions

sync from upstream main to feat 3 2

xuzhg mentioned this pull request Oct 27, 2017

Modify the serialization for each element #41

Closed

PerthCharern reviewed Oct 27, 2017

View reviewed changes

darrelmiller reviewed Oct 30, 2017

View reviewed changes

xuzhg added 5 commits October 30, 2017 12:15

add the write functionality for each open api element

a1b5328

arrange the test files into correct folder

a6ee229

Add OpenApiConstant.cs and add the Unit test for OpenApiContact

20f5d47

Modify and add test for OpenApiInfo, OpenApiLicense

7b48c99

remove unused files (OpenApiSerializer, OpenApiV2(3)Writer, etc and c…

1b4632c

…odes

rebase to the latest master codes

27a8742

xuzhg commented Oct 30, 2017

View reviewed changes

make WriteAsV2(3) as internal function in the DOM

25b2c36

change the Yaml writer for the empty object write

c6caee2

PerthCharern reviewed Oct 30, 2017

View reviewed changes

xuzhg added 3 commits October 30, 2017 15:53

Change the error messages and move them into project resource

0cdae13

Add the IOpenApiWriter back to public api because the WriteAsV2(3) as…

079e24c

… internal

change the comments in the serialize public api

06841d9

PerthCharern reviewed Oct 30, 2017

View reviewed changes

resolve the typo and change the StringExtensions.cs

2bc14da

PerthCharern approved these changes Oct 30, 2017

View reviewed changes

modify the OpenApiSecurityScheme and OpenApiOAuthFlow

6e40cc6

xuzhg merged commit d062eb7 into microsoft:master Oct 31, 2017

xuzhg deleted the AddWriteForElement branch October 31, 2017 00:01

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

Merge pull request #42 from BinkyLabs/chore/main-to-feat-3-2

0f84c3e

sync from upstream main to feat 3 2


		expectedYaml = expectedYaml.MakeLineBreaksEnvironmentNeutral();

		// Assert

Conversation

xuzhg commented Oct 27, 2017

Uh oh!

xuzhg commented Oct 27, 2017

Uh oh!

xuzhg commented Oct 27, 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!

xuzhg Oct 27, 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 27, 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!

Choose a reason for hiding this comment

Uh oh!

xuzhg commented Oct 27, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

darrelmiller commented Oct 30, 2017

Uh oh!

xuzhg commented Oct 30, 2017

Uh oh!

darrelmiller commented Oct 30, 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!

darrelmiller Oct 30, 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!

xuzhg commented Oct 30, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PerthCharern Oct 30, 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!

xuzhg Oct 30, 2017 • edited by PerthCharern 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!

xuzhg commented Oct 30, 2017

Uh oh!

xuzhg Oct 27, 2017 •

edited

Loading

xuzhg commented Oct 30, 2017 •

edited

Loading

darrelmiller Oct 30, 2017 •

edited

Loading

PerthCharern Oct 30, 2017 •

edited

Loading

xuzhg Oct 30, 2017 •

edited by PerthCharern

Loading

xuzhg Oct 30, 2017 •

edited by PerthCharern

Loading