Skip to content

Conversation

scott-lin
Copy link
Contributor

Introducing a property to track the specification version read, so consumers of the Reader have this metadata and do not have to parse it themselves. See #172

doc = this.VersionService.LoadDocument(this.RootNode);
diagnostic.SpecificationVersion = OpenApiSpecVersion.OpenApi3_0;
}
else
Copy link
Contributor

@xuzhg xuzhg Jan 11, 2018

Choose a reason for hiding this comment

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

The else if (...) { } and else { ...} has the same codes? why cannot we merge them together?

@PerthCharern @darrelmiller #WontFix

Copy link
Contributor

Choose a reason for hiding this comment

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

The "else" is for a default case. It is, but doesn't have to be, the same as the "else if". This makes sure we list out the cases for 2.0 and 3.0 explicitly.

If you are worried about code duplication, we can refactor the guts out.


In reply to: 160842325 [](ancestors = 160842325)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Won't fix with the scope of this change.


In reply to: 160843959 [](ancestors = 160843959,160842325)

@scott-lin scott-lin changed the title [Fix issue #172]: Adding spec version to OpenApiDiagnostic of Reader Fix #172: Adding spec version to OpenApiDiagnostic of Reader Jan 11, 2018
@PerthCharern
Copy link
Contributor

PerthCharern commented Jan 11, 2018

I'd rather put these two tests in the same folder:

test/Microsoft.OpenApi.Readers.Tests/OpenApiReaderTests

The ComparisonTests in V2Tests should probably be moved out to this folder as well. #Resolved

diagnostic.Should().NotBeNull();
diagnostic.SpecificationVersion.Should().Be( OpenApiSpecVersion.OpenApi3_0 );
}
}
Copy link
Contributor

@PerthCharern PerthCharern Jan 11, 2018

Choose a reason for hiding this comment

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

nit, spacing #Resolved

var callback = subscribeOperation.Callbacks["simpleHook"];

context.ShouldBeEquivalentTo(new OpenApiDiagnostic());
context.ShouldBeEquivalentTo(
Copy link
Contributor

@PerthCharern PerthCharern Jan 11, 2018

Choose a reason for hiding this comment

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

context [](start = 16, length = 7)

If you have some time, would you mind fixing these context -> diagnostic? If not, it's okay. We can do a big cleanup later. #Resolved

Copy link
Contributor

@PerthCharern PerthCharern left a comment

Choose a reason for hiding this comment

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

:shipit:

@scott-lin scott-lin merged commit db4c009 into master Jan 11, 2018
@scott-lin scott-lin deleted the scottlin_issue172 branch January 11, 2018 01:57
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