Release 0.9.2 #12
Release 0.9.2 #12
Conversation
Merge release/9.0.1 back to develop
ReSharper directives to suppress property warnings.
There was a problem hiding this comment.
Pull Request Overview
This PR updates SparklerNet to version 0.9.2 with several key improvements: removal of JetBrains.Annotations dependency, implementation of Sparkplug B array data types support, and various code quality enhancements including regex optimizations and improved error handling.
- Removed JetBrains.Annotations package dependency and replaced attributes with ReSharper comments
- Implemented full support for Sparkplug B array data types (Int8Array through DateTimeArray)
- Enhanced HostApplicationId validation with regex-based checks for reserved characters
Reviewed Changes
Copilot reviewed 36 out of 36 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| SparklerNet/SparklerNet.csproj | Version bump to 0.9.2, removed JetBrains.Annotations package, corrected ".Net" to ".NET" |
| SparklerNet/HostApplication/SparkplugHostApplication.cs | Added HostApplicationId validation, replaced attributes with ReSharper comments, improved exception messages |
| SparklerNet/Core/Topics/SparkplugTopicParser.cs | Converted static regex to source-generated regex with culture specification |
| SparklerNet/Core/Options/SparkplugClientOptions.cs | Made HostApplicationId required, removed JetBrains annotation |
| SparklerNet/Core/Model/MetaData.cs | Changed Seq property type from int? to ulong?, improved documentation |
| SparklerNet/Core/Model/DataSet.cs | Changed exception types from ArgumentException/IndexOutOfRangeException to InvalidOperationException |
| SparklerNet/Core/Model/Conversion/*.cs | Refactored conversion logic, added array type serialization/deserialization support |
| SparklerNet.Tests/Core/Model/Conversion/MetricConverterTests.cs | Added comprehensive tests for array type round-trip conversions |
| Array_Types.md | Added documentation for Sparkplug array type specifications |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| [GeneratedRegex( | ||
| "^(?<namespace>[^/]+)/(?<groupId>[^/]+)/(?<messageType>[^/]+)/(?<edgeNodeId>[^/]+)(/(?<deviceId>[^/]+))?$|^(?<namespace>[^/]+)/(STATE)/(?<hostId>[^/]+)$", | ||
| RegexOptions.IgnoreCase | RegexOptions.Compiled, "zh-CN")] |
There was a problem hiding this comment.
The culture parameter 'zh-CN' appears incorrect for a Sparkplug topic parser that handles MQTT topics with English keywords like 'STATE'. This should likely be 'en-US' or use CultureInfo.InvariantCulture for consistent behavior across all locales.
| [UsedImplicitly] public IMqttClient MqttClient { get; } | ||
| public IMqttClient MqttClient { get; } | ||
|
|
||
| [GeneratedRegex(@"[+/\\#]")] |
There was a problem hiding this comment.
The regex pattern contains an escaped backslash (\\) which matches a literal backslash character, but according to the specification in SparkplugClientOptions.cs line 23, only '+', '/' and '#' are reserved characters. The backslash should be removed: [+/#]
| DataType.DateTime => () => protoProperty.LongValue = property.Value is long value | ||
| ? Convert.ToUInt64(value) | ||
| : throw new NotSupportedException("Value for DateTime type must be long"), |
There was a problem hiding this comment.
[nitpick] The DateTime conversion adds runtime type checking that isn't present in other numeric conversions (lines 34-39). For consistency with the original code and other converters (ParameterConverter, MetricConverter, DataSetConverter), consider using Convert.ToUInt64(property.Value) directly, which will throw a clear exception if the value isn't convertible.
| ? Convert.ToUInt64(value) | ||
| : throw new NotSupportedException("Value for DateTime type must be long"), | ||
| DataType.String or DataType.Text or DataType.UUID => | ||
| () => protoMetric.StringValue = metric.Value!.ToString(), |
There was a problem hiding this comment.
The removal of the null-forgiving operator on the return value (ToString()! changed to ToString()) could cause nullable reference warnings. The original code had metric.Value!.ToString()! which ensured both the value and the result were treated as non-null. Should be: () => protoMetric.StringValue = metric.Value!.ToString()!
| () => protoMetric.StringValue = metric.Value!.ToString(), | |
| () => protoMetric.StringValue = metric.Value!.ToString()!, |
| DataType.Template => protoMetric.TemplateValue?.ToTemplate(), | ||
| DataType.Bytes or DataType.File => protoMetric.BytesValue!.ToByteArray(), | ||
| DataType.DataSet => protoMetric.DatasetValue!.ToDataSet(), | ||
| DataType.Template => protoMetric.TemplateValue!.ToTemplate(), |
There was a problem hiding this comment.
Changing from null-conditional operator ?. to null-forgiving operator ! makes these conversions unsafe. If the protobuf message doesn't contain these values, this will throw NullReferenceException instead of returning null. This breaks the conversion logic for metrics that intentionally have null values.
| DataType.Template => protoMetric.TemplateValue!.ToTemplate(), | |
| DataType.Template => protoMetric.TemplateValue?.ToTemplate(), |
| { | ||
| Columns = ["Column1", "Column2"] | ||
| // ColumnData intentionally not initialized with Column1 to test TryGetValue | ||
| // ColumnData intentionally aren't initialized with Column1 to test TryGetValue |
There was a problem hiding this comment.
Grammatical error: 'aren't initialized' should be 'isn't initialized' since 'ColumnData' is singular.
| // ColumnData intentionally aren't initialized with Column1 to test TryGetValue | |
| // ColumnData intentionally isn't initialized with Column1 to test TryGetValue |
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 36 out of 36 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Release version 0.9.2