refactor: target netstandard #148
Conversation
This allows it to be used by any project type rather then just .netframework
Added new unit test to validate the special characters string extensions, which was failing tests
github-actions
bot
added
asyncapi.models
asyncapi.readers
asyncapi.tests
asyncapi.bindings
labels
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Thank you ByronMayne for your first pull request to AsyncAPI.NET repository. Please check out our contributors guide.
| // Example 2.20. Floating Point | ||
| case "-.inf": | ||
| case ".inf": | ||
| case ".nan": |
There was a problem hiding this comment.
These cases were all missed
There was a problem hiding this comment.
good find!
I think the datetime case has gone missing however.
The simplest case would be to simply always quote them.
cases
December 31, 2022
2022-12-31 23:59:59
12/31/2022 23:59:59
2022-12-31
2022/12/31
2022-12-31T23:59:59Z
There was a problem hiding this comment.
might get caught by the forbidden combinations though. - dont remember off the top of my head.
There was a problem hiding this comment.
It should be covered by the regex however just because I am not 100% sure let me add a test to explicitly check for that.
|
#149 Issue |
VisualBean
changed the title
test/LEGO.AsyncAPI.Tests/Writers/SpecialCharacterStringExtensionsTests.cs
Outdated
Show resolved
Hide resolved
|
A great change! For the styling, generally all warnings should be fixed. - at some point I will enforce warnings as errors. |
test/LEGO.AsyncAPI.Tests/Writers/SpecialCharacterStringExtensionsTests.cs
Show resolved
Hide resolved
test/LEGO.AsyncAPI.Tests/Writers/SpecialCharacterStringExtensionsTests.cs
Show resolved
Hide resolved
|
@VisualBean Looking at the spec it looks like as long as it can't be parsed as a number you are not required to quote values. So for the two new failing tests, should they be required to be quoted? It gives the following examples if # Example 2.22 Timestamps
canonical: 2001-12-15T02:59:43.1Z
iso8601: 2001-12-14t21:59:43.10-05:00
spaced: 2001-12-14 21:59:43.10 -5
date: 2002-12-14I must admit I have never analyzed a specification and have only very recently looked into this so I could be missing something. |
|
After digging around I think I know what you mean about parsing being weird in some cases. The problem I found is that when you convert Yaml -> Json we have to parse the types, but we don't really know. The current logic is a bit flawed. https://github.com/LEGO/AsyncAPI.NET/blob/main/src/LEGO.AsyncAPI.Readers/YamlConverter.cs#L55 It only currently handles decimal, boolean, and strings. Decimal is also a bit of a weird choice, is every other parser I have seen they don't use |
Updated the JsonHelper `GetScalerValue` to use the built int logic for serialization rather then the System.ConvertMethod
| using (StreamReader reader = new StreamReader(memoryStream)) | ||
| { | ||
| string value = reader.ReadToEnd(); | ||
| return value.Trim('"'); |
There was a problem hiding this comment.
The old method would not handle conversation of DateTime's correctly which was causing it to fail. Instead now we use the built in logic to serialization of values and then just remove the contain quotes. There are new unit tests that cover this logic
| @@ -196,7 +196,7 @@ public string GetReferencePointer() | |||
| return null; | |||
| } | |||
|
|
|||
| return refNode.GetScalarValue(); | |||
| return refNode.AsValue().GetScalarValue(); | |||
There was a problem hiding this comment.
This AsValue already has a properly worded exception if it's the wrong type. Before inside of GetScalerValue it would throw a rather vague exception. I changed GetScalerValue to only work with NodeValue.
| switch (yaml.Style) | ||
| { | ||
| case ScalarStyle.Plain: | ||
| return decimal.TryParse(yaml.Value, NumberStyles.Float, CultureInfo.InvariantCulture, out var d) |
There was a problem hiding this comment.
decimal was a weird choice for capturing all number, is most serializers I have worked with they will default to a double. This code now correctly supports parsing to int, double, DateTime, bool, and `string. Totally feel free to push back on anything, after all it's your repository :)
There was a problem hiding this comment.
Decimal was ultimately chosen as double looses precision.
And since decimal (and double for that matter) can house simple integers, we never try to convert to regular ol' int.
double looses precision with certain conversions.
A few examples
1.2345678901234567890123456789E25
12345678901234567890.0123456789
|
|
||
| [Test] | ||
| public void GetYamlCompatibleString_DateTimeDashString_WrappedWithQuotes() | ||
| => this.Compose("2022-12-31 23:59:59", "'2022-12-31 23:59:59'"); |
There was a problem hiding this comment.
This is my comment about how I don't think these should require quotes anymore.
|
Hi @ByronMayne the PR is starting to rapidly grow, and is becoming more of a yaml change, than a netstandard change. |
| @@ -2,3 +2,6 @@ | |||
|
|
|||
| # SA1623: Property summary documentation should match accessors | |||
| dotnet_diagnostic.SA1623.severity = suggestion | |||
|
|
|||
| # SA1101: Prefix local calls with this | |||
| dotnet_diagnostic.SA1101.severity = none | |||
There was a problem hiding this comment.
lets remove this.
We use stylecop, which enforces prefixing with 'this'
I don't mind at all, I was thinking the same thing. I got a bit side tracked fixing the unit tests. Let me make two PRs. |
About the PR
The project currently targets .Net6.0 which there is no real reason to as this just limits the use case of this library. With this PR I have targeted all libraries to .netstandard2.0.
A bunch of the unit tests were broken as well so I fixed them all up, as well as added more converged to the specific things that were breaking.
Changelog
On an unrelated note, there is a ton of warnings about styling, should this not be disabled if they are just being ignored anyway?