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

Fixing Mail deserialization issue. #225

Merged
merged 4 commits into from
Nov 28, 2017

Conversation

sccalabr
Copy link
Contributor

@sccalabr sccalabr commented Oct 1, 2017

@thinkingserious thinkingserious added status: code review request requesting a community code review or review from Twilio difficulty: hard fix is hard in difficulty hacktoberfest labels Oct 1, 2017
@TBuc
Copy link
Contributor

TBuc commented Oct 3, 2017

@thinkingserious have you ever considered adopting Project Lombok library in order to widely reduce boilerplate coding?
Implementing standard, all-parameters equals and hashcode methods would reduce to something as simple as a class annotation: @EqualsAndHashCode (ref. official Lombok doc for @EqualsAndHashCode).

@thinkingserious
Copy link
Contributor

Hi @TBuc,

This is the first I've heard of it, thanks for the heads up!

Would that be something that makes this PR easier?

@TBuc
Copy link
Contributor

TBuc commented Oct 4, 2017

Hi @thinkingserious,

actually yes, it would make the PR easier as, in example:

PR for file BccSettings.java would reduce to just two lines of code:

import lombok.EqualsAndHashCode;
@EqualsAndHashCode
public class BccSettings {

PR for file Mail.java would require two lines but a little more effort since I can see you are not including all its parameters in the equals() and compareTo() methods. You will then need to create a list of the parameters to be excluded (I will just list part of them for this example):

import lombok.EqualsAndHashCode;
@EqualsAndHashCode(exclude={"personalization","content"})
public class Mail {

That said, I embrace (and love) lombok, but you should take a minute to decide whether to adopt it or not. As any library, it has its wild sides too (even thought I did not find any in this specific annotation yet), and a build goal should be added for providing delomboking of classes, also in order to be able to check the code it produces for you.

@thinkingserious
Copy link
Contributor

Hi @TBuc,

Let's revisit this after we get this one done. Your feedback there is greatly appreciated as well :)

Thanks!

With Best Regards,

Elmer

@mbernier
Copy link
Contributor

I cleaned up the merge conflicts and pushed in all the recent changes

@thinkingserious
Copy link
Contributor

Hi @sccalabr,

Could you please resolve these merge conflicts? Sorry about the hassle :(

With Best Regards,

Elmer

@thinkingserious thinkingserious merged commit 4d58fcb into sendgrid:master Nov 28, 2017
@thinkingserious
Copy link
Contributor

Hello @sccalabr,

Thanks again for the PR!

We appreciate your contribution and look forward to continued collaboration. Thanks!

Team SendGrid DX

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
difficulty: hard fix is hard in difficulty status: code review request requesting a community code review or review from Twilio
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants