Skip to content
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

Implement Email Notifications #61

Merged
merged 18 commits into from
Feb 5, 2024
Merged

Implement Email Notifications #61

merged 18 commits into from
Feb 5, 2024

Conversation

vicr123
Copy link
Owner

@vicr123 vicr123 commented Feb 4, 2024

This PR implements a framework for email notifications.

Closes #11
Closes #12
Closes #14

@vicr123 vicr123 requested a review from reflectronic February 5, 2024 02:41
Parlance.Notifications/Parlance.Notifications.csproj Outdated Show resolved Hide resolved
Comment on lines +143 to +158
// Get static properties using Reflection
var autoSubscriptionEventNameProperty = autoSubscriptionType.GetProperty("AutoSubscriptionEventName", BindingFlags.Public | BindingFlags.Static);
var channelNameProperty = channelType.GetProperty("ChannelName", BindingFlags.Public | BindingFlags.Static);

// Check if the properties exist
if (autoSubscriptionEventNameProperty == null || channelNameProperty == null)
{
throw new ArgumentException("The required static properties do not exist in the provided types.");
}

// Get the values of the static properties
var autoSubscriptionEventName = (string)autoSubscriptionEventNameProperty.GetValue(null!)!;
var channelName = (string)channelNameProperty.GetValue(null!)!;

// Call the original function
return await GetAutoSubscriptionPreference(channelName, autoSubscriptionEventName, userId);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should call the generic overload using MakeGenericMethod

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This (and the other files like it) really feel like they want to be scoped services that are automatically resolved and invoked by some "main" hosted service that pulls from the message bus... we might want to see if MessagePipe has some integration with MS.Ext.DI, or maybe switch to a library that does

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could be rewritten to use symbols instead of syntax

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This source generator (and the other ones) should ideally be using IIncrementalGenerator instead of ISourceGenerator. The legacy model is deprecated and can cause performance problems in the IDE

@vicr123 vicr123 marked this pull request as ready for review February 5, 2024 10:21
@vicr123 vicr123 merged commit 88c2df6 into main Feb 5, 2024
2 checks passed
@vicr123 vicr123 deleted the features/notifications branch February 5, 2024 10:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Notification Settings Notifications Notifications for translation freezes
2 participants