-
-
Notifications
You must be signed in to change notification settings - Fork 70
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
Improved types for generated service arguments #1149
Improved types for generated service arguments #1149
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1149 +/- ##
====================================
Coverage 82% 82%
====================================
Files 190 190
Lines 3779 3781 +2
Branches 425 425
====================================
+ Hits 3107 3115 +8
+ Misses 507 499 -8
- Partials 165 167 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
@@ -46,7 +46,7 @@ public void TestSomeBasicServicesCanBeParsed() | |||
[Fact] | |||
public void TestServicesWithAdvancedFieldsCanBeParsed() | |||
{ | |||
var sample = File.ReadAllText(@"CodeGenerator/ServiceMetaDataSamples/Lights.json"); | |||
var sample = File.ReadAllText(@"CodeGenerator/ServiceMetaDataSamples/light.json"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would a raw string literal be a better way to store the json string? Then you don't have to do any file IO in the tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For these very long ones it seems to me it clutters the test to put them inline. That is arbitrary of course
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could put them in a separate class as constants (especially when these are used in multiple tests)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it is fine keep them in a file, the tests are not really taking that long.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me, I think this breaking change is manageable and adds enough value
@@ -46,7 +46,7 @@ public void TestSomeBasicServicesCanBeParsed() | |||
[Fact] | |||
public void TestServicesWithAdvancedFieldsCanBeParsed() | |||
{ | |||
var sample = File.ReadAllText(@"CodeGenerator/ServiceMetaDataSamples/Lights.json"); | |||
var sample = File.ReadAllText(@"CodeGenerator/ServiceMetaDataSamples/light.json"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it is fine keep them in a file, the tests are not really taking that long.
* Improved types for generated service arguments * fix test
Improved the parameter types for some generated service arguments. specifically:
date => DateOnly
time => TimeOnly
datetime => DateTime
rgb_color => IReadOnlyCollection,
number without 'step' is now double instead of long
This will allow for eg:
Light.Attic.TurnOn(transition: 0.5, rgbColor: [255,128,10]);
Proposed change
Type of change
Additional information
Checklist
If user exposed functionality or configuration are added/changed: