Skip to content

Conversation

@MattApril
Copy link

Here is a re-factored version of the SDK that I wrote.

The new base object APIResource can be easily extended and modeled for various endpoints, or used directly on any unwrapped endpoints (see provided example).

I will admit I'm pushing these changes a bit prematurely, but I am out of town for a week so I tried to quickly clean it up and get it out before I left. It has NOT been tested, outside of the unit tests. Once I get back I will continue development.

@richleland
Copy link
Contributor

Hi @MattApril sorry for the delayed response here! Thanks for the contribution. We're going to be reviewing this over the next few days.

@bdeanindy
Copy link
Contributor

While running the tests for this, I received the following error (not sure if Github is honoring the tests for the PR or not).

There was 1 failure:

1) SparkPost\Test\SparkPostTest::testGetConfigEmptyException
Failed asserting that exception of type "Exception" is thrown.

FAILURES!
Tests: 43, Assertions: 68, Failures: 1.

The logic for that method appears to be alright, but I am going to continue testing and playing with this some more.

@MattApril
Copy link
Author

I missed that one because I ran the tests individually, and you will find that SparkPostTest passes when run alone.

The reason no exception is thrown is because the config is infact set (see TransmissionTest::setUp). So I would chalk it up to an error in the test; it should clear the config before calling GetConfig, rather than assuming that it is empty.

@MattApril
Copy link
Author

Committed some minor changes to allow the failing unit test to pass.

@richleland Any updates on the status of this pull request? I will be working on a project over the next month or two using this SDK, so I will be actively contributing and hopefully build this into a fairly complete library.

@beardyman
Copy link
Contributor

@MattApril: Just wanted to give you an update since its been a while. I'm in the process of reviewing this. I'm hoping to get some feedback to you early this week.

beardyman added a commit that referenced this pull request Sep 9, 2015
SDK Refactor for easy extensibility
@beardyman beardyman merged commit aeea514 into SparkPost:master Sep 9, 2015
@richleland
Copy link
Contributor

Thanks for the awesome contribution @MattApril - feel free to submit a PR to add yourself to AUTHORS.md - and let us know how that project goes!

@beardyman beardyman mentioned this pull request Sep 15, 2015
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.

4 participants