Use records with Bh1745 #1258
Use records with Bh1745 #1258
Conversation
ghost
added
the
area-device-bindings
| @@ -39,6 +39,10 @@ | |||
| </Reference> | |||
| </ItemGroup> | |||
|
|
|||
| <ItemGroup Condition="'$(TargetFramework)' != 'netstandard2.0'"> | |||
There was a problem hiding this comment.
Shouldn't this condition be the other way around? Right now it is compiling the file on net5.0 but not on NS2.0
There was a problem hiding this comment.
Discussed offline, we will instead move this type to iot.device.bindings and the binding itself and try to make it internal.
src/devices/Bh1745/Bh1745.cs
Outdated
| { | ||
| Red = 2.2, Green = 1.0, Blue = 1.8, Clear = 10.0 | ||
| }; | ||
| ChannelCompensationMultipliers = new ( |
There was a problem hiding this comment.
While I don't have a big preference, I suppose that object initializer still works with records right? Like:
ChannelCompensationMultipliers = new ()
{
Red = 2.2, Green = 1.0, Blue = 1.8, Clear = 10.0
};There was a problem hiding this comment.
If I do that, then the constructor isn't satisfied, and then I cannot use the pretty records syntax.
src/devices/Bh1745/Bh1745.csproj
Outdated
| @@ -8,6 +8,7 @@ | |||
|
|
|||
| <ItemGroup> | |||
| <Compile Include="*.cs" /> | |||
| <Compile Include="$(MSBuildThisFileDirectory)/../Common/System/Runtime/CompilerServices/IsExternalInit.cs"/> | |||
There was a problem hiding this comment.
| <Compile Include="$(MSBuildThisFileDirectory)/../Common/System/Runtime/CompilerServices/IsExternalInit.cs"/> | |
| <Compile Include="$(MSBuildThisFileDirectory)/../Common/System/Runtime/CompilerServices/IsExternalInit.cs"> | |
| <Link>Common/System/Runtime/CompilerServices/IsExternalInit.cs</Link> | |
| </Compile> |
Nit: Add a Link here for VS Experience.
There was a problem hiding this comment.
If you switch the order of the TFMs in Bh1745.csproj or load these projects on a machine within .NET 5.0, you will get the following experience, with IsExternalInit.cs listed. Just ignore it.
* Add records to Bh1745: * Add tracking issue for /// docs * Move record to primary class file * Define records first * Simplify sleep * Add extension file * Add object initializers * Remove space * Change extension method name * Move IsExternalInit * Update handling of IsExternalInit
First test of using records in this codebase. This binding seemed like a perfect use case for records.
I made a few choices:
IsExternalInitas part of thenetstandard2.0asset (required to use records pre .NET 5.0).These are all a first take. What do you think?
///doc comments don't currently work for records due to dotnet/roslyn#44571. I don't think this should stop us from getting started with records.Going forward, I think we should convert a small set of data-oriented classes to records, and see if we get any feedback on their usage. For this first case, I've gone with the (very pretty) immutable version of records. That seems to fit this case well. We may find that we want to enable mutable properties for some cases. That said, the
withsyntax is pretty compelling.