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

Issue 16 Adding a base object #27

Merged
merged 14 commits into from
Apr 22, 2015
Merged

Issue 16 Adding a base object #27

merged 14 commits into from
Apr 22, 2015

Conversation

aydrian
Copy link
Contributor

@aydrian aydrian commented Apr 17, 2015

Please look at this carefully. I would like as many eyes on it as possible. I've changed things around. The configuration library is gone. We now have a base client object with contained configs. I used the ExactTarget and Twilio node SDKs as inspiration. I also switched to using the nock library instead of our custom mocks.
Looking for suggestions to resolve this

lib/index.js
  line 22  col 25  This function's cyclomatic complexity is too high. (9)

Once this is in place, I should be able to quickly implement the rest of the APIs.
I appreciate all questions, suggestions, thoughts, limericks, what have you.

@aydrian
Copy link
Contributor Author

aydrian commented Apr 17, 2015

Also, would like to get your thoughts about how we are handling the response from the server in the individual API's. The ones in transmissions were wrapped and sending domains were not. I usually just pass the server response straight through. I want to be consistent with this. Appreciate your thoughts.


// we need options
if( !_.isPlainObject( options ) ) {
throw new TypeError( 'options argument is required' );
Copy link
Contributor

Choose a reason for hiding this comment

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

This error might want to mention that options needs to be an object?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Completely doable. I'm not the best at coming up with error messages. First thought was just to make them all return "seg fault". 😏

@jasonrhodes
Copy link
Contributor

As for cycolmatic complexity, it doesn't look like that method is all that complex... I'd probably start by pulling this out into some kind of options = handleOptions(apiKey, options); type method to see if that helps

// only options passed in
if (typeof apiKey === 'object') {
options = apiKey;
this.apiKey = process.env.SPARKPOST_API_KEY;
} else {
options = options || {};
this.apiKey = apiKey || process.env.SPARKPOST_API_KEY;
}

this.request( options, callback );
};

SparkPost.prototype['delete'] = function( options, callback ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

isn't javascript fun :)

@jmartin4563
Copy link
Contributor

Just wanted to add as a general comment, I've noticed that there's some spacing going inside of function argument declarations, which we don't usually do. Like:

var foo = function( bar, bax, bat ) {
  // does stuff
};

@aydrian
Copy link
Contributor Author

aydrian commented Apr 17, 2015

Sorry, the spacing was beaten into me at my last job.

aydrian added a commit that referenced this pull request Apr 22, 2015
Issue 16 Adding a base object. Closes #16
@aydrian aydrian merged commit 52960f5 into SparkPost:master Apr 22, 2015
@aydrian aydrian deleted the ISSUE-16 branch May 7, 2015 12:02
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