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

getMethod option #221

Merged
merged 4 commits into from Jun 2, 2016
Merged

getMethod option #221

merged 4 commits into from Jun 2, 2016

Conversation

@tameraydin
Copy link
Contributor

tameraydin commented May 20, 2016

Introduces getMethod option to Twitter provider in order to allow using different methods from its rest API.

@@ -4,6 +4,8 @@ exports = module.exports = function (options) {

options = options || {};

This comment has been minimized.

Copy link
@ldesplat

ldesplat May 23, 2016

Contributor

Let's clean this up a bit now that it's getting slightly more complex

const defaults = {
    extendedProfile: true,
    getMethod: 'users/show'
};
const settings = Hoek.applyToDefaults(defaults, options);

Don't forget to require hoek and change options to settings.

@@ -20,7 +22,7 @@ exports = module.exports = function (options) {
return callback();
}

get('https://api.twitter.com/1.1/users/show.json', { user_id: params.user_id }, (profile) => {
get('https://api.twitter.com/1.1/' + getMethod + '.json', { user_id: params.user_id }, (profile) => {

This comment has been minimized.

Copy link
@ldesplat

ldesplat May 23, 2016

Contributor

ES6 String

get(`https://api.twitter.com/1.1/${settings.getMethod}.json`, ...
@ldesplat

This comment has been minimized.

Copy link
Contributor

ldesplat commented May 23, 2016

Looks good to me, Just these two minor comments. Thanks, now it makes more sense what you were asking for.

@ldesplat ldesplat added this to the 7.8.0 milestone May 23, 2016
@gergoerdosi

This comment has been minimized.

Copy link

gergoerdosi commented May 23, 2016

I think the name getMethod is a bit confusing. Method has a meaning in requests and this setting has nothing to do with that.

@tameraydin

This comment has been minimized.

Copy link
Contributor Author

tameraydin commented May 25, 2016

@ldesplat thanks for the feedback.. I did the changes you suggested but now somehow I'm getting this weird "'equal' of undefined" error on CI (when I try locally -node v4,5 or 6-, all tests are passing, no fail). Do you have any idea?

@gergoerdosi

This comment has been minimized.

Copy link

gergoerdosi commented May 25, 2016

@tameraydin: Remove deep in tests, it's the default in code v3.0.0.

@ldesplat ldesplat modified the milestones: 7.8.0, 7.7.2 May 26, 2016
@tameraydin

This comment has been minimized.

Copy link
Contributor Author

tameraydin commented May 26, 2016

@gergoerdosi thanks, it helped! ..but I'm curious, why the assertion I added was the only one failing, I see that deep comparison is used in all other provider specs & assertions?

@gergoerdosi

This comment has been minimized.

Copy link

gergoerdosi commented May 26, 2016

I don't see deep being used anywhere in the code. Which files are you referring to?

@tameraydin

This comment has been minimized.

Copy link
Contributor Author

tameraydin commented May 26, 2016

ahh my bad.. my fork branch wasn't up-to-date:) thx!

@ldesplat

This comment has been minimized.

Copy link
Contributor

ldesplat commented Jun 2, 2016

Thank you @gergoerdosi for the consistent review. And thank you @tameraydin for contributing!

@ldesplat ldesplat merged commit e75ce80 into hapijs:master Jun 2, 2016
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.