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

Change NullValueHandling to Include #113 #114

Closed

Conversation

richardszalay
Copy link

See #113 for details.

This doesn't actually require a change to the Skips_null_values test, since it was only asserting a null value (and not an absent property)

@feinoujc
Copy link
Owner

feinoujc commented May 8, 2020

Thanks for the PR!

I'm not sure I want to change the setting on a global level just for the merge var content, but I wonder if there is a way to target the content specifically, what if we added an overload when adding a merge variable?

message.AddGlobalMergeVars("profile", new { FirstName = null }, new JsonSerializerSettings { NullValueHandling = NullValueHandling.Include });

This would allow people to customize the way their merge var content gets serialized further, I guess this gets converted to snake_case today, for example

@richardszalay
Copy link
Author

I like the idea, but it feels weird to provide an override for all merge vars while adding a single var. Perhaps a separate method?

@feinoujc
Copy link
Owner

feinoujc commented May 9, 2020

good point @richardszalay. We can create a JsonConverter for the MandrillMergeVar type and serialize the content that way. I have this working over in #116

I am curious if there is a need to expose the ability to serialize the content in other custom ways (naming conventions, how dates are serialized etc...) differently than the mandrill conventions? Thoughts?

Right now, the way to do that is to custom models for content and use attributes:

public class MyContent {

   [JsonProperty("firstName")] // otherwise it will be first_name
   public string FirstName { get; set; }

   [JsonConverter(typeof(IsoDateTimeConverter))] // otherwise it will be a unix timestamp
   [JsonProperty("returnDate")]
   public DateTime ReturnDate { get;set;}
}

@richardszalay
Copy link
Author

I don't think there's a strong need to customise things like dates because, as you say, you can just customise the model.

@feinoujc
Copy link
Owner

Resolved in #116

@feinoujc feinoujc closed this May 10, 2020
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.

2 participants