-
Notifications
You must be signed in to change notification settings - Fork 14
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
Make the Personalization’s dynamicTemplateData generic #17
Make the Personalization’s dynamicTemplateData generic #17
Conversation
This should allow support for nested Codable structures, and better type support when encoded to JSON. Fixes vapor-community#7
@@ -21,8 +21,8 @@ public struct Personalization: Codable { | |||
public var substitutions: [String: String]? | |||
|
|||
/// A collection of key/value pairs following the pattern "key":"value" to substitute handlebar template data | |||
public var dynamicTemplateData: [String: String]? | |||
|
|||
public var dynamicTemplateData: DynamicTemplateData? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This technically breaks public API correct? @tonyarnold
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it does, sadly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright we'll just have new major versions of Kit and the helper library.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry! Without this change, though, it's not really possible to use SendGrid's dynamic templates properly.
Seems worth it? 😄
@Andrewangeta what are the chances of this PR making it into a release (even a tagged beta) at some point soon? Did you need to see any changes to the code? |
@tonyarnold Hey! Sorry for the delay. Would you be ok/comfortable taking over and maintaining this repo? I have a TON on my plate constantly and don't want to hinder growth and development of the ecosystem because I'm too busy. I actually don't even use sendgrid myself so I'd much rather someone else with more invested take over and be a contributor and shape the evolution of the package overtime. Just LMK and you can take the reigns and do what's bets for this library :) |
@Andrewangeta I'd be happy to help in whatever way makes sense. I'm happy to look after the library for the time being, and if you find yourself with time/inclination in future just let me know. |
@Andrewangeta are there any expectations, rules, etc around maintaining this package? (and, I assume the parent https://github.com/vapor-community/sendgrid package). If I want to make a release, are there docs on how to go about that? |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #17 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 9 9
Lines 104 137 +33
=========================================
+ Hits 104 137 +33
|
This should allow support for nested Codable structures, and better type support when encoded to JSON (everything needed to be a string, so things like numbers, nil and booleans were not being encoded properly.
It does, sadly, make the declaration of Personalization instances a bit messier. I'm open to approaches here.
Fixes #7