Heavy cleanup/abstractions#29
Conversation
This commit heavily refactored the value classes. Please check the attached 'ValueClasses.cd' in the SparkplugNet Project or use this link: https://i.imgur.com/94iHzvf.jpg - Heavily cleaned/refactored PayloadConverter (number of lines reduced by almost 60%) - Removed MetricBase - Added ValueBase class as base for all shared data classes - Added ValueBase class children for Sparkplug Version A and B - Added DataTypeExtensions class for converting data types (decoupled it from MetricBase) - Removed Value property from IMetric (IValue takes care of this now) - Added interface IValue for value classes Changed some names for consistency in Value types for A and B: - Renamed 'Bool' to 'Boolean' - Renamed 'Integer' to 'Int' - Renamed 'Type' to 'DataType' - Added/Renamed ValueCase (uint) and DataType (Enum) as properties for IValue (used this renaming for consistency purposes. Some classes used ValueCase as Enum before). I think ‘Value’Case is better suited for the underlying value (uint) and DataType for the Enum as it shows the data type in text. - Added Bytes field for KuraMetric - Added DataSet helper constructor - Added DataSetValue helper constructors - Metric, DataSetValue, Parameter and PropertyValue now inherit from ValueBaseVersionB
|
I am against breaking changes on this large scale |
| /// <summary> | ||
| /// Gets the value. | ||
| /// </summary> | ||
| public object? Value { get; } |
There was a problem hiding this comment.
why do you remove that from the Interface?
I'm not completely against the changes, but need to check what has changed first. But in general, I agree. The project should now get into a state where the basic "API" should get more stable, I guess. |
Please take a look at the Class Diagram, I moved it to IValue. I see now that this merge request did not display the link (updated now). The build-up is different, made to adhere to the DRY rule. Instead of having intValue, floatValue, etc. in 5 different classes, it has now been abstracted. What large-scale breaking changes are you talking about? The only changes to the 'public' are the consistent namings of Boolean in the DataType enums, and consistency in the ValueCase and DataType properties. 99% of the changes I made are under the hood. |
You are right - sorry i just did a quick browser over your code. |
|
I hope to check this tomorrow, let's see. |
|
This looks good. I first thought that you changed code from the ProtoBuf generated classes (Which would have been bad), but unifiing the internally used metrics seems to be a very good idea. |
|
@all-contributors please add @Patrick2607 for infrastructure, tests and code |
|
I've put up a pull request to add @Patrick2607! 🎉 |
3 participants
This commit heavily refactored the value classes. Please check the attached 'ValueClasses.cd' in the SparkplugNet Project or view the Class Diagram directly.
Heavily cleaned/refactored PayloadConverter (number of lines reduced by almost 60%)
Removed MetricBase
Added ValueBase class as base for all shared data classes
Added ValueBase class children for Sparkplug Version A and B
Added DataTypeExtensions class for converting data types (decoupled it from MetricBase)
Removed Value property from IMetric (IValue takes care of this now)
Added interface IValue for value classes
Changed some names for consistency in Value types for A and B:
Renamed 'Bool' to 'Boolean'
Renamed 'Integer' to 'Int'
Renamed 'Type' to 'DataType'
Added/Renamed ValueCase (uint) and DataType (Enum) as properties for IValue (used this renaming for consistency purposes. Some classes used ValueCase as Enum before). I think ‘Value’Case is better suited for the underlying value (uint) and DataType for the Enum as it shows the data type in text.
Added Bytes field for KuraMetric
Added DataSet helper constructor
Added DataSetValue helper constructors
Metric, DataSetValue, Parameter and PropertyValue now inherit from ValueBaseVersionB