Fix package name and add static String for Content-Type #148
Conversation
|
Thanks for the pull request @marcosbarbero! Could you please take a moment to sign our CLA so we can merge it? https://github.com/sendgrid/sendgrid-java/blob/master/CONTRIBUTING.md#cla The tests will not pass on Travis for you because the environment variable the tests depends on is not available to those outside of SendGrid. I'll test locally to make sure all is good. Thanks! |
thinkingserious
added
type: community enhancement
|
@thinkingserious Thanks, I'll sign to CLA in few! |
|
Just sent my CLA agreement |
|
@thinkingserious this second commit should be a second PR, just messed things up! hehehe |
|
Hello @marcosbarbero, This issue finally bubbled up to the top of our queue. Could you please resolve the conflicts so we can review for merge? Thank you! |
|
@thinkingserious thanks for taking a look on it, tomorrow morning I'll fix it and re-delivery! |
|
@thinkingserious it took longer than I thought but here is the review. |
There was a problem hiding this comment.
Hi @marcosbarbero!
This PR finally reached the top of our backlog :)
Please take a look at my feedback and let me know what you think.
| * @param apiKey is your SendGrid API Key: https://app.sendgrid.com/settings/api_keys | ||
| * @param client the Client to use (allows to customize its configuration) | ||
| */ | ||
| public SendGrid(String apiKey, Client client) { |
There was a problem hiding this comment.
So that we don't have to make a breaking change, could you please keep this constructor?
| initializeSendGrid(apiKey); | ||
| } | ||
|
|
||
| public void initializeSendGrid(String apiKey) { |
There was a problem hiding this comment.
Please keep this function also, so that we don't have a breaking change. Generally, please ensure all public functions remain and just add a comment that notes that we will deprecate for now.
|
It looks like we have a conflict that needs resolving as well. Please let me know if there is anything I can do to help move this forward. Thanks! |
|
|
|
@thinkingserious I just updated the PR |
|
Thanks for fixing the conflicts! Could you please also take a look at my change requests? Thanks! |
|
@thinkingserious can you check them? As far as I can see I rollbacked all the breaking changes |
|
Hi @marcosbarbero, It looks like this function was removed: public void initializeSendGrid(String apiKey) |
|
@thinkingserious I'm so sorry.. Just rollbacked this one |
|
No problem @marcosbarbero, thanks for the quick fix. |
|
Hi @marcosbarbero, Could you please remove the edits from the following files? We will update these after the merge: CHANGELOG.md Thanks! |
thinkingserious
added
the
status: work in progress
|
Hello @marcosbarbero, Just checking in. Is there anything I can do to help? |
|
Would it be fine for me to fork the changes & update it so it can be merged in? It'd make contributing to other bugs (#181) much easier for me |
|
Sorry for this long delay. Unfortunately I don't have much time to work on this issue. |
|
@marcosbarbero Sure, if you don't mind |
|
Just transfered @hc1jcarter |
|
Am I doing something wrong on the tests? They all pass for me locally but TravisCI seems to be having problems... |
|
Hi @hc1jcarter, You are good. We have to update TravisCI. Right now, it only can read the environment variables when the tests originate from a SendGrid team account. Here is the relevant ticket for the fix. With Best Regards, Elmer |
|
fixed #159 for oracle java 8... still trying to work on the java 7 issues. They seem unrelated to the code itself and more on the travis-ci environment |
|
Implementation will be tracked in this issue. |
This PR fix #141