Skip to content

Further exponential notation fix. #581

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

Conversation

EricWilson-BluePrism
Copy link
Contributor

I neglected to apply the NumberStyles.Float fix for supporting exponential notation to the V2 SchemaDeserializer. I also added it to the handling of Maximum and Minimum properties in both V2 and V3 SchemaDeserializer's as hey both leverage decimal.Parse().

…ntial notation to the V2 SchemaDeserializer. I also added it to the handling of Maximum and Minimum properties.
@EricWilson-BluePrism
Copy link
Contributor Author

@darrelmiller any idea what's causing my pull requests to fail on the OpenApi-CI-Allbranches check? This is the second time. Unfortunately, the check won't let me see the details.

@darrelmiller
Copy link
Member

The tests are failing...

[xUnit.net 00:00:05.51]   Starting:    Microsoft.OpenApi.Readers.Tests
##[error][xUnit.net 00:00:07.09]     Microsoft.OpenApi.Readers.Tests.V2Tests.OpenApiDocumentTests.ParseDocumentWithDifferentCultureShouldSucceed(culture: "hi-IN") [FAIL]
[xUnit.net 00:00:07.09]       System.FormatException : Input string was not in a correct format.
[xUnit.net 00:00:07.10]       Stack Trace:
[xUnit.net 00:00:07.10]            at System.Number.StringToNumber(String str, NumberStyles options, NumberBuffer& number, NumberFormatInfo info, Boolean parseDecimal)
[xUnit.net 00:00:07.10]            at System.Number.ParseDecimal(String value, NumberStyles options, NumberFormatInfo numfmt)
[xUnit.net 00:00:07.10]            at System.Decimal.Parse(String s, NumberStyles style, IFormatProvider provider)
[xUnit.net 00:00:07.10]         src\Microsoft.OpenApi.Readers\V2\OpenApiSchemaDeserializer.cs(35,0): at Microsoft.OpenApi.Readers.V2.OpenApiV2Deserializer.<>c.<.cctor>b__83_141(OpenApiSchema o, ParseNode n)
[xUnit.net 00:00:07.10]         src\Microsoft.OpenApi.Readers\ParseNodes\PropertyNode.cs(41,0): at Microsoft.OpenApi.Readers.ParseNodes.PropertyNode.ParseField[T](T parentInstance, IDictionary`2 fixedFields, IDictionary`2 patternFields)
[xUnit.net 00:00:07.10]         src\Microsoft.OpenApi.Readers\V2\OpenApiSchemaDeserializer.cs(252,0): at Microsoft.OpenApi.Readers.V2.OpenApiV2Deserializer.LoadSchema(ParseNode node)
[xUnit.net 00:00:07.10]         src\Microsoft.OpenApi.Readers\ParseNodes\MapNode.cs(70,0): at Microsoft.OpenApi.Readers.ParseNodes.MapNode.<>c__DisplayClass6_0`1.<CreateMap>b__0(KeyValuePair`2 n)
[xUnit.net 00:00:07.10]            at System.Linq.Enumerable.WhereSelectEnumerableIterator`2.MoveNext()

…es.Float, but they are fine with NumberStyles.Any. NumberStyles.Any includes the exponential style, so we're good.
@EricWilson-BluePrism
Copy link
Contributor Author

Thank you Darrel. I'll blame it on a "senior" moment. Test are passing now.

@darrelmiller
Copy link
Member

@EricWilson-BluePrism I believe your code was right and the tests are wrong. JSON and YAML do not allow commas in integer or floating point values. Yaml does support octal and hexadecimal but JSON doesn't, so we shouldn't. The commas should be removed from the tests and the NumberStyle should go back to being Float.

Let me know if you want me to make those change.

@EricWilson-BluePrism
Copy link
Contributor Author

Hi @darrelmiller,

I'm happy to make the changes. Do you want me to update the test cases too?

Cheers,
Eric

…different cultures. Also reverted the NumberStyles change from Any back to Float.
@EricWilson-BluePrism
Copy link
Contributor Author

@darrelmiller,

I've changed the NumberStyles back to Float and removed the commas from the two test cases.

Cheers,
Eric

@darrelmiller darrelmiller merged commit 9e609bf into microsoft:vnext May 1, 2021
@darrelmiller
Copy link
Member

Thank you for this contribution. I'm really hoping we can get the build pipeline sorted so that we can release an update soon.

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.

2 participants