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

Make the Personalization’s dynamicTemplateData generic #17

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

tonyarnold
Copy link
Collaborator

@tonyarnold tonyarnold commented Jul 4, 2023

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

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?
Copy link
Member

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, it does, sadly.

Copy link
Member

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.

Copy link
Collaborator Author

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? 😄

@tonyarnold
Copy link
Collaborator Author

@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?

@Andrewangeta
Copy link
Member

@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 :)

@tonyarnold
Copy link
Collaborator Author

@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.

@tonyarnold
Copy link
Collaborator Author

@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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Nested structure personalizations
2 participants