Skip to content

Modify the serialization for each element#41

Closed
xuzhg wants to merge 1 commit intomicrosoft:masterfrom
xuzhg:OpenApiSerialization
Closed

Modify the serialization for each element#41
xuzhg wants to merge 1 commit intomicrosoft:masterfrom
xuzhg:OpenApiSerialization

Conversation

@xuzhg
Copy link
Contributor

@xuzhg xuzhg commented Oct 26, 2017

  1. add the extension methods for each element.
  2. remove OpenApiV2(3)Writer
  3. remove OpenApiV2(3)Serializer
  4. Add some Unit test.
  5. All other tests pass

1. add the extension methods for each element.
2. remove OpenApiV2(3)Writer
3. remove OpenApiV2(3)Serializer
4. Add some Unit test.
5. All other tests pass
@xuzhg xuzhg requested a review from darrelmiller October 26, 2017 23:19
@xuzhg
Copy link
Contributor Author

xuzhg commented Oct 26, 2017

@PerthCharern

/// <summary>
/// Extensions method for <see cref="OpenApiCallback"/> serialization.
/// </summary>
internal static class OpenApiCallbackExtensions
Copy link
Contributor

Choose a reason for hiding this comment

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

OpenApiCallbackExtensions [](start = 26, length = 25)

I like it that we are finally separating out these Serialize methods into their own classes, instead of one giant serializer.

I'm reading through this, but at first glance, I wonder this: Is it beneficial to have these as Extension methods? Have you considered putting the methods directly into the models?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

:) I am working the whole morning to covert it to as you said.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The PR is almost ready.

Copy link
Member

Choose a reason for hiding this comment

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

Sorry for missing this, I was flying all day Friday. I guess we can discuss this further at our meeting today.

{
""description"": ""Find more info here"",
""url"": ""https://example.com""
}";
Copy link
Contributor

Choose a reason for hiding this comment

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

Once we have more of these tests (which I think we should in order to test the whole serialization), this literal string is going to be very distracting.

I suggest we read this from a file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What I am thinking is that the Unit test can test every small part individually.

for example, we can test the "OpenApiTag" serialization individually, same as the "OpenApiDocument".

But, I will do a little bit small changes in the coming PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

That sounds like a good idea. This is fine then. I'd say we should still have at least a couple of test that verify that the overall serialization works well.

@PerthCharern
Copy link
Contributor

    public bool IsV3 { get { return Version == "V3"; } set { Version = "V3"; } }

These booleans are kinda funny. I'd rather make format and version a public property with getter and setter.


Refers to: src/Microsoft.OpenApi.Workbench/MainModel.cs:65 in 6884b00. [](commit_id = 6884b00, deletion_comment = False)

@xuzhg
Copy link
Contributor Author

xuzhg commented Oct 27, 2017

@PerthCharern

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

for the MainModel.cs, we can leave it later when we back to the test & bench project.

@xuzhg
Copy link
Contributor Author

xuzhg commented Oct 30, 2017

As we discussed today to take the #42, so close it.

@xuzhg xuzhg closed this Oct 30, 2017
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

Comments