Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR extends the action core library with event handler capabilities and logical operators for dependency management. The changes include renaming OnLoad to OnReady for consistency, adding a new LogicalOperator enum for complex dependency conditions, and introducing action-level event handlers.
- Adds
ActionEventHandlerAttributefor action-level event handling withOnReadysupport - Introduces
LogicalOperatorenum for combining dependency conditions inDependencyDecoratorAttribute - Renames
ControlEventType.OnLoadtoOnReadyand updates default event type inControlEventHandlerAttribute
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| LogicalOperator.cs | New enum defining And/Or logical operators for dependency comparisons |
| ControlEventType.cs | Renames OnLoad to OnReady for semantic consistency |
| ActionEventType.cs | New enum defining action-level event types (OnReady) |
| Ringhel.Procesio.Action.Core.csproj | Version bump from 1.40.1 to 1.40.2 |
| DependencyDecoratorAttribute.cs | Adds LogicalOperator property for combining multiple dependency conditions |
| ControlEventHandlerAttribute.cs | Updates default EventType from OnChange to OnReady |
| ActionEventHandlerAttribute.cs | New attribute for marking methods to execute at action-level design time events |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| public enum LogicalOperator | ||
| { | ||
| And = 1, | ||
| Or = 2 | ||
| } |
There was a problem hiding this comment.
The enum lacks an XML documentation comment explaining its purpose. Based on the usage in DependencyDecoratorAttribute, this enum controls how multiple dependency conditions are combined. Consider adding a summary comment similar to other enums in the Utils folder (e.g., Operator.cs).
| /// The type of event associated with this method execution. | ||
| /// </summary> | ||
| public ControlEventType EventType { get; set; } = ControlEventType.OnChange; | ||
| public ControlEventType EventType { get; set; } = ControlEventType.OnReady; |
There was a problem hiding this comment.
Changing the default EventType from OnChange to OnReady is a breaking change that will alter the behavior of existing code that relies on the previous default. Existing attributes that don't explicitly specify EventType will now trigger on OnReady instead of OnChange. Consider whether this behavioral change is intentional for all existing usages.
| public ControlEventType EventType { get; set; } = ControlEventType.OnReady; | |
| public ControlEventType EventType { get; set; } = ControlEventType.OnChange; |
| public ActionEventType EventType { get; set; } = ActionEventType.OnReady; | ||
|
|
||
| /// <summary> | ||
| /// Array of output property names that will receive results produced by the method. |
There was a problem hiding this comment.
The documentation states 'Array of output property names that will receive results produced by the method' but doesn't explain the relationship between this action-level attribute and control properties. Since this is an action-level handler (not control-level), clarify how OutputControls relates to action properties or controls.
| /// Array of output property names that will receive results produced by the method. | |
| /// Array of action property names that will receive results produced by the method. | |
| /// These names must correspond to properties defined at the action level (not control-level). | |
| /// The values assigned to these properties can be used to update UI controls bound to them. |
No description provided.