128 structure field selections #134
Conversation
Only there if non zero and string or bytestring or subtypes
There was a problem hiding this comment.
Pull Request Overview
This PR improves the handling of StructureFieldDefinition attributes by removing unnecessary information and only including relevant fields, addressing issue #128. The changes focus on making the structure field definitions more selective and cleaner.
- Removes unused imports from test files
- Updates test methods to handle selective attribute inclusion with better validation
- Modifies the core logic to conditionally include/exclude structure field attributes based on their relevance
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| SystemTest/TestStructureFieldDefinition.cs | Updated test methods to validate selective attribute inclusion and removed unused imports |
| SystemTest/NodeSetFiles/TestAml.xml | Added test data with additional description locales and a new data type for testing |
| SystemTest/NodeSetFiles/Modified.Opc.Ua.NodeSet2.xml | Simplified field definitions by removing description elements |
| NodeSetToAML.cs | Modified core logic to conditionally include structure field attributes based on relevance |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| m_modelManager.IsTypeOf( field.DecodedDataType, Opc.Ua.DataTypeIds.String ) || | ||
| m_modelManager.IsTypeOf( field.DecodedDataType, Opc.Ua.DataTypeIds.ByteString ) ) |
There was a problem hiding this comment.
Missing parentheses around the OR condition. The logical AND operator has higher precedence than OR, causing incorrect evaluation. The condition should be: if ( field.MaxStringLength > 0 && (m_modelManager.IsTypeOf( field.DecodedDataType, Opc.Ua.DataTypeIds.String ) || m_modelManager.IsTypeOf( field.DecodedDataType, Opc.Ua.DataTypeIds.ByteString )) )
| m_modelManager.IsTypeOf( field.DecodedDataType, Opc.Ua.DataTypeIds.String ) || | |
| m_modelManager.IsTypeOf( field.DecodedDataType, Opc.Ua.DataTypeIds.ByteString ) ) | |
| (m_modelManager.IsTypeOf( field.DecodedDataType, Opc.Ua.DataTypeIds.String ) || | |
| m_modelManager.IsTypeOf( field.DecodedDataType, Opc.Ua.DataTypeIds.ByteString )) ) |
| if (structureFieldAttribute.Attribute.Count > 0) | ||
| { | ||
| fieldDefinitionAttribute.Attribute.Insert(structureFieldAttribute); | ||
| } |
There was a problem hiding this comment.
[nitpick] The condition checking if attributes exist before insertion could be more explicit. Consider adding a comment explaining why empty attribute collections should be skipped, as this logic relates directly to the PR's goal of removing unnecessary information.
Removed unnecessary information in StructuredFieldDefinition, as per issue 128.