-
Notifications
You must be signed in to change notification settings - Fork 4
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Added event handler for MemoryManager
operations.
#59
Conversation
We could simplify the code by sending the current instance of MemoryManager (i.e., 'this') as the sender when the event is raised. I notice that in most cases, the description of the generated event (MemoryStorageEventEnum.Upsert.GetEnumDescription()) is currently being sent as the sender. While I understand that there may be a specific reason for this, it's worth mentioning to ensure clarity :) |
src/Encamina.Enmarcha.SemanticKernel.Abstractions/Events/MemoryStorageEventEnum.cs
Outdated
Show resolved
Hide resolved
src/Encamina.Enmarcha.SemanticKernel.Abstractions/Events/MemoryStorageEventExtensions.cs
Outdated
Show resolved
Hide resolved
src/Encamina.Enmarcha.SemanticKernel.Abstractions/Events/MemoryStorageEventEnum.cs
Outdated
Show resolved
Hide resolved
src/Encamina.Enmarcha.SemanticKernel/Extensions/IServiceCollectionExtensions.cs
Outdated
Show resolved
Hide resolved
Perhaps add comments in the code to explain how this event is expected to work and be consumed. For instance, that the name of the event is sent to allow handlers to filter. |
There is no reason for this. Your arguments are logical and the event type is sent in the arguments, so it doesn't make sense to send the event type in the sender. I modify it to send the sender correctly. |
MemoryManager
operations.