Skip to content

Refactoring, correct package and fluent interface #149

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 12 commits into from

Conversation

RichardCSantana-zz
Copy link

Refactoring some class.
Implement fluent interface.
Correcting some packages.

@thinkingserious
Copy link
Contributor

Hello @richardcsantana,

Thanks for the pull request!

Before we can merge it, we need a signed CLA: https://github.com/sendgrid/sendgrid-java/blob/master/CONTRIBUTING.md#cla. Please get that over to us at [email protected] when you can.

With Best Regards,

Elmer

@RichardCSantana-zz
Copy link
Author

@thinkingserious I signed it and sent the email.

@thinkingserious
Copy link
Contributor

@richardcsantana,

Your issue has come up for pull request review. Can you please resolve the conflicts? Thanks!

@RichardCSantana-zz
Copy link
Author

@thinkingserious Done

@thinkingserious
Copy link
Contributor

Awesome, thank you!

A couple of requests:

  1. Can you please update the README examples to reflect the changes: https://github.com/sendgrid/sendgrid-java/blob/master/README.md
  2. Can you please list any breaking changes (for example, https://github.com/sendgrid/sendgrid-java/pull/149/files#diff-b17f74d9628458cd62154f4635856988R1)

@RichardCSantana-zz
Copy link
Author

RichardCSantana-zz commented Oct 20, 2016

Breaking change, only the imports that I correct, I think it could be on the changelog, so I'll put it here if you don't mind to put it on changelog:

Change import com.sendgrid.Mail to import com.sendgrid.helpers.mail.Mail
change import com.sendgrid.ASM to import com.sendgrid.helpers.mail.objects.ASM                         
change import com.sendgrid.ClickTrackingSetting to import com.sendgrid.helpers.mail.objects.ClickTrackingSetting
change import com.sendgrid.FooterSetting to import com.sendgrid.helpers.mail.objects.FooterSetting
change import com.sendgrid.OpenTrackingSetting to import com.sendgrid.helpers.mail.objects.OpenTrackingSetting
change import com.sendgrid.SpamCheckSetting to import com.sendgrid.helpers.mail.objects.SpamCheckSetting
change import com.sendgrid.Attachments to import com.sendgrid.helpers.mail.objects.Attachments
change import com.sendgrid.Content to import com.sendgrid.helpers.mail.objects.Content
change import com.sendgrid.GoogleAnalyticsSetting to import com.sendgrid.helpers.mail.objects.GoogleAnalyticsSetting
change import com.sendgrid.Personalization to import com.sendgrid.helpers.mail.objects.Personalization
change import com.sendgrid.SubscriptionTrackingSetting to import com.sendgrid.helpers.mail.objects.SubscriptionTrackingSetting
change import com.sendgrid.BccSettings to import com.sendgrid.helpers.mail.objects.BccSettings
change import com.sendgrid.Email to import com.sendgrid.helpers.mail.objects.Email
change import com.sendgrid.MailSettings to import com.sendgrid.helpers.mail.objects.MailSettings
change import com.sendgrid.Setting to import com.sendgrid.helpers.mail.objects.Setting
change import com.sendgrid.TrackingSettings to import com.sendgrid.helpers.mail.objects.TrackingSettings

@thinkingserious
Copy link
Contributor

Awesome @richardcsantana!

@thinkingserious
Copy link
Contributor

Hello @richardcsantana,

Could you please resolve the conflicts? Thanks!

@SendGridDX
Copy link
Collaborator

SendGridDX commented Apr 12, 2017

CLA assistant check
Thank you for your submission, we really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
5 out of 6 committers have signed the CLA.

✅ dmitraver
✅ thinkingserious
✅ RichardCSantana
✅ rafalwrzeszcz
✅ SendGridDX
❌ richardcssantana
You have signed the CLA already but the status is still pending? Let us recheck it.

@thinkingserious
Copy link
Contributor

@dmitraver,

Do you mind signing our new CLA? It only requires a short form and a click of a button now :)

@dmitraver
Copy link
Contributor

dmitraver commented Apr 12, 2017 via email

@thinkingserious
Copy link
Contributor

@richardcsantana,

Could you please resolve the above conflicts? Please don't worry about any versioning, we will take care of that.

Thanks!

With Best Regards,

Elmer

@RichardCSantana-zz
Copy link
Author

Done!

@thinkingserious
Copy link
Contributor

@richardcsantana,

In the file test/java/com/sendgrid/helpers/AttachmentBuilderTest.java, please replace:

import com.sendgrid.Attachments;

with

import com.sendgrid.helpers.mail.objects.Attachments;

Copy link
Contributor

@thinkingserious thinkingserious left a comment

Choose a reason for hiding this comment

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

Generally I see a lot of formatting changes. Could you please review: https://github.com/sendgrid/sendgrid-java/blob/master/CONTRIBUTING.md#style_guidelines_and_naming_conventions

Thanks!

@@ -126,6 +129,40 @@ public class Example {
}
```

or using fluence interface
Copy link
Contributor

Choose a reason for hiding this comment

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

*fluent

.setFrom(new Email("[email protected]"))
.setSubject("Hello World from the SendGrid Java Library!")
.addPersonalization(new Personalization()
.addTo(new Email("[email protected]")))
Copy link
Contributor

Choose a reason for hiding this comment

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

The indentation is off here

@thinkingserious thinkingserious added the status: work in progress Twilio or the community is in the process of implementing label Apr 29, 2017
@thinkingserious
Copy link
Contributor

HI @richardcsantana,

Just checking in. Is there anything I can do to help?

@thinkingserious
Copy link
Contributor

Implementation will be tracked in this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: work in progress Twilio or the community is in the process of implementing type: community enhancement feature request not on Twilio's roadmap
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants