Effect alias #114
Effect alias #114
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR introduces an "alias" feature for effects, allowing effects to be referenced by a human-readable name in addition to their system-generated IDs. The changes enable effects to be looked up and manipulated using either their unique EffectId or a string alias.
Key changes:
- Added an optional
Aliasparameter to theStoredEffecttype and its factory methods - Modified
Capturemethods to use implicit IDs internally while treating the provided string as an alias - Updated lookup logic in
ExistingEffectsandEffectResultsto support searching by both ID and alias - Refactored test assertions to use alias-based lookups instead of ID-based lookups
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| Core/Cleipnir.ResilientFunctions/Storage/Types.cs | Added alias parameter to StoredEffect factory methods |
| Core/Cleipnir.ResilientFunctions/Domain/ExistingEffects.cs | Implemented ID-or-alias lookup logic for effect operations |
| Core/Cleipnir.ResilientFunctions/Domain/EffectResults.cs | Added alias support to effect capture and resolution methods |
| Core/Cleipnir.ResilientFunctions/Domain/Effect.cs | Changed Capture methods to use implicit IDs with alias parameter |
| Core/Cleipnir.ResilientFunctions.Tests/TestTemplates/FunctionTests/EffectTests.cs | Updated test assertions to use alias-based lookups |
| Core/Cleipnir.ResilientFunctions.Tests/TestTemplates/FunctionTests/AtMostOnceWorkStatusTests.cs | Updated test to pass string as alias parameter |
| Core/Cleipnir.ResilientFunctions.Tests/InMemoryTests/StoredEffectSerializationTests.cs | Added explicit alias: null to all StoredEffect.Create* calls |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| // Then try to find by alias | ||
| var storedEffect = storedEffects.FirstOrDefault(e => e.Value.Alias == effectId); | ||
| if (storedEffect.Value == null) |
There was a problem hiding this comment.
Checking storedEffect.Value == null is incorrect for KeyValuePair<TKey, TValue>. When FirstOrDefault finds no match, it returns default(KeyValuePair<...>), which has a non-null Value of type StoredEffect (a record). Instead, check storedEffect.Equals(default(KeyValuePair<EffectId, StoredEffect>)) or use storedEffects.Values.FirstOrDefault(...).
| if (storedEffect.Value == null) | |
| if (storedEffect.Equals(default(KeyValuePair<EffectId, StoredEffect>))) |
|
|
||
| // Then try to find by alias | ||
| var storedEffect = storedEffects.FirstOrDefault(e => e.Value.Alias == effectId); | ||
| if (storedEffect.Value == null) |
There was a problem hiding this comment.
Same issue as Comment 1: checking storedEffect.Value == null is incorrect for KeyValuePair<TKey, TValue>. When FirstOrDefault returns no match, it returns default(KeyValuePair<...>), not a pair with a null value. Use storedEffect.Equals(default(KeyValuePair<EffectId, StoredEffect>)) or query storedEffects.Values.FirstOrDefault(...).
| if (storedEffect.Value == null) | |
| if (storedEffect.Equals(default(KeyValuePair<EffectId, StoredEffect>))) |
No description provided.