Skip to content

Update serializer settings to include default values #273 #275

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

Closed
wants to merge 7 commits into from
Closed

Update serializer settings to include default values #273 #275

wants to merge 7 commits into from

Conversation

owre
Copy link
Contributor

@owre owre commented Jul 14, 2016

Changed json serializer settings to include default values to fix issue #273
The default implementation removes the enable flags from tracking settings if set to false. Resulting in:
The click_tracking(etc) enable parameter is required.

@thinkingserious
Copy link
Contributor

Awesome!

Could you please sign our CLA to get this merged? https://github.com/sendgrid/sendgrid-csharp/blob/master/CONTRIBUTING.md#cla

Thanks!

@owre
Copy link
Contributor Author

owre commented Jul 15, 2016

I have signed the CLA and sent it to dx@
Cheers

@thinkingserious
Copy link
Contributor

I will need to give this one some more thought.

The failing TestHelloEmail test happens because your change allows "send_at":0 to be a part of the returned JSON object because the default value for a long is zero. We need to ignore unset values, but in C# all variables are initialized.

I'm thinking we need to instead build the object piece by piece and then deserialize that object vs trying to deserialize the entire Mail object and figuring out which values to ignore properly.

@owre
Copy link
Contributor Author

owre commented Jul 20, 2016

How about making it nullable? That way it wont be included.

@thinkingserious
Copy link
Contributor

I was thinking about trying this: https://msdn.microsoft.com/en-us/library/53b8022e(v=vs.110).aspx

@thinkingserious
Copy link
Contributor

I like the nullable idea also.

@owre
Copy link
Contributor Author

owre commented Jul 20, 2016

Good idea http://www.newtonsoft.com/json/help/html/conditionalproperties.htm

I think a nullable property would be easier. Is it any more property that should not be serialized with its default value?

@thinkingserious
Copy link
Contributor

Generally, if you don't set a value, then it should not show up.

@thinkingserious
Copy link
Contributor

Seems like if we go the nullable route, we need to initialize the values to null. Looking at the failed test, it looks like we still get "send_at":0

@owre
Copy link
Contributor Author

owre commented Jul 20, 2016

I missed to make sendAt in mail nullable. Fixed :)

@thinkingserious
Copy link
Contributor

Nice work! I'm hoping to have some time to review today.

@thinkingserious
Copy link
Contributor

@owre,

I just put in a pull request on your fork that adds in the rest of the nullable fixes, plus an additional unit test.

Sync with Master + Add nullable fix to remainder of Mail.cs + Add Unit Test
thinkingserious added a commit that referenced this pull request Aug 1, 2016
@thinkingserious
Copy link
Contributor

#293

@thinkingserious
Copy link
Contributor

@owre,

Please shoot us an email to [email protected] with your mailing address and T-shirt size so we can send you a token of our appreciation.

@owre
Copy link
Contributor Author

owre commented Aug 6, 2016

Thank you very much! :)

gabrielkrell pushed a commit to gabrielkrell/sendgrid-csharp that referenced this pull request Aug 2, 2017
Add 'Sending email with attachment' Use Case
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: bug bug in the library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants