Enhance OptionObject and RowObject helpers with locking and unlocking…#295
Conversation
… functionality - Added methods to lock and unlock fields in OptionObject2 and OptionObject. - Introduced validation for field numbers to ensure they are not null or empty. - Improved error handling for cases where no matching field objects are found. - Refactored existing methods to reduce code duplication and improve readability. - Updated documentation to reflect new methods and their usage.
- Introduced a new ArgumentGuards class to centralize validation logic for field numbers and field objects. - Replaced inline validation code in OptionObject2Helpers, OptionObjectHelpers, and RowObjectHelpers with calls to ArgumentGuards methods. - Removed redundant constant messages from helper classes, utilizing those defined in ArgumentGuards instead. - Improved code readability and maintainability by consolidating validation logic.
There was a problem hiding this comment.
Pull request overview
This PR enhances the *.Helpers layer by adding lock/unlock operations for FieldObject across RowObject, FormObject, and the OptionObject* variants, while centralizing argument validation and updating unit tests/documentation to reflect the new behavior.
Changes:
- Added
SetLocked*/SetUnlocked*helper methods forRowObject,FormObject,OptionObject,OptionObject2, andOptionObject2015. - Introduced a shared
ArgumentGuardsutility for consistent validation/normalization of field numbers and field object lists. - Refactored tests by splitting OptionObject helper tests per type and updating expectations to match the new exception-based validation behavior.
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| dotnet/RarelySimple.AvatarScriptLink.Objects.Helpers/Validators/ArgumentGuards.cs | New shared guard/normalization helpers used by the extension methods. |
| dotnet/RarelySimple.AvatarScriptLink.Objects.Helpers/RowObjectHelpers.cs | Adds per-field and multi-field lock/unlock helpers; changes missing-field behavior to throw for these operations. |
| dotnet/RarelySimple.AvatarScriptLink.Objects.Helpers/FormObjectHelpers.cs | Adds lock/unlock helpers and applies centralized field number validation + “no matches” error handling. |
| dotnet/RarelySimple.AvatarScriptLink.Objects.Helpers/OptionObjectHelpers.cs | Adds lock/unlock helpers and refactors enable/disable to use ArgumentGuards. |
| dotnet/RarelySimple.AvatarScriptLink.Objects.Helpers/OptionObject2Helpers.cs | Same as above for OptionObject2. |
| dotnet/RarelySimple.AvatarScriptLink.Objects.Helpers/OptionObject2015Helpers.cs | Same as above for OptionObject2015. |
| dotnet/RarelySimple.AvatarScriptLink.Objects.Helpers.Tests/ArgumentGuardsTests.cs | New unit tests for ArgumentGuards. |
| dotnet/RarelySimple.AvatarScriptLink.Objects.Helpers.Tests/RowObjectHelpersTests.cs | Updates test naming + adds coverage for new lock/unlock behavior and new guard-based exception cases. |
| dotnet/RarelySimple.AvatarScriptLink.Objects.Helpers.Tests/FormObjectHelpersTests.cs | Updates test naming + adds coverage for new lock/unlock behavior and new guard-based exception cases. |
| dotnet/RarelySimple.AvatarScriptLink.Objects.Helpers.Tests/OptionObjectHelpersTests.cs | Removes the combined OptionObject/OptionObject2/OptionObject2015 test file (replaced by per-type files). |
| dotnet/RarelySimple.AvatarScriptLink.Objects.Helpers.Tests/OptionObjectHelpers.OptionObject.Tests.cs | New OptionObject-specific helper tests (including lock/unlock). |
| dotnet/RarelySimple.AvatarScriptLink.Objects.Helpers.Tests/OptionObjectHelpers.OptionObject2.Tests.cs | New OptionObject2-specific helper tests (including lock/unlock). |
| dotnet/RarelySimple.AvatarScriptLink.Objects.Helpers.Tests/OptionObjectHelpers.OptionObject2015.Tests.cs | New OptionObject2015-specific helper tests (including lock/unlock). |
| .github/instructions/dotnet.instructions.md | Documents the test naming pattern and the per-type OptionObject test file organization. |
Comments suppressed due to low confidence (3)
dotnet/RarelySimple.AvatarScriptLink.Objects.Helpers/OptionObjectHelpers.cs:252
SetDisabledFieldchecks that any form contains the field number, but then callsform.SetDisabledField(fieldNumber)for every form. With the newFormObject.SetDisabledFieldbehavior (throws when the field is not present in that form), this will throw as soon as the OptionObject contains a form that does not include the field. Consider iterating only over forms whereIsFieldPresent(fieldNumber)(same applies toSetEnabledField,SetLockedField,SetUnlockedField).
if (!optionObject.Forms.Any(f => f.IsFieldPresent(fieldNumber)))
throw new ArgumentException(ArgumentGuards.NoMatchingFieldObjectsMessage, nameof(fieldNumber));
foreach (var form in optionObject.Forms)
{
form.SetDisabledField(fieldNumber);
}
dotnet/RarelySimple.AvatarScriptLink.Objects.Helpers/OptionObject2Helpers.cs:82
SetDisabledFieldvalidates that at least one form contains the field number, but then invokesform.SetDisabledField(fieldNumber)on every form. BecauseFormObject.SetDisabledFieldnow throws when the field doesn't exist in that form, this will fail when the OptionObject2 includes forms without that field. Consider only calling into forms whereIsFieldPresent(fieldNumber)(same applies toSetEnabledField,SetLockedField,SetUnlockedField).
if (!optionObject.Forms.Any(f => f.IsFieldPresent(fieldNumber)))
throw new ArgumentException(ArgumentGuards.NoMatchingFieldObjectsMessage, nameof(fieldNumber));
foreach (var form in optionObject.Forms)
{
form.SetDisabledField(fieldNumber);
}
dotnet/RarelySimple.AvatarScriptLink.Objects.Helpers/OptionObject2015Helpers.cs:102
SetDisabledFieldchecks that at least one form contains the field number, but then callsform.SetDisabledField(fieldNumber)for every form. With the updatedFormObject.SetDisabledFieldthrowing when the field doesn't exist in that form, this will throw for OptionObject2015 instances that contain other forms without that field. Consider only calling into forms whereIsFieldPresent(fieldNumber)(same applies toSetEnabledField,SetLockedField,SetUnlockedField).
if (!optionObject.Forms.Any(f => f.IsFieldPresent(fieldNumber)))
throw new ArgumentException(ArgumentGuards.NoMatchingFieldObjectsMessage, nameof(fieldNumber));
foreach (var form in optionObject.Forms)
{
form.SetDisabledField(fieldNumber);
}
dotnet/RarelySimple.AvatarScriptLink.Objects.Helpers/OptionObjectHelpers.cs
Show resolved
Hide resolved
dotnet/RarelySimple.AvatarScriptLink.Objects.Helpers/OptionObject2Helpers.cs
Outdated
Show resolved
Hide resolved
dotnet/RarelySimple.AvatarScriptLink.Objects.Helpers/OptionObject2015Helpers.cs
Outdated
Show resolved
Hide resolved
… in OptionObject and OptionObject2 helpers
|



Enhance OptionObject and RowObject helpers with locking and unlocking functionality