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

Add trakt.tv provider #321

Merged
merged 12 commits into from Nov 19, 2017
Merged

Add trakt.tv provider #321

merged 12 commits into from Nov 19, 2017

Conversation

@marcuspoehls
Copy link
Contributor

marcuspoehls commented Sep 11, 2017

The provider to support the trakt.tv OAuth API is yet missing. This PR helps to add support for trakt.

It uses the extended trakt.tv user profile by default.

Hopefully this helps users to connect with trakt.tv’s OAuth API :)

Copy link
Contributor

ldesplat left a comment

Your own trackt.tv test fails with

Debug: internal, implementation, error 
    Error: Uncaught error: `value` required in setHeader("trakt-api-key", value).
    at ClientRequest.OutgoingMessage.setHeader (_http_outgoing.js:354:11)
    at new ClientRequest (_http_client.js:79:14)
    at Object.exports.request (http.js:31:10)
    at internals.Client.request.options.beforeRedirect.onResponse [as request] 

If you solve that, we can get it merged.

@ldesplat ldesplat added the feature label Sep 24, 2017
@marcuspoehls

This comment has been minimized.

Copy link
Contributor Author

marcuspoehls commented Sep 25, 2017

@ldesplat Ups, sorry. Will check and correct the tests. Thank you

@AdriVanHoudt

This comment has been minimized.

Copy link
Contributor

AdriVanHoudt commented Oct 12, 2017

Coverage: 99.94% (1/1601)
lib/providers/trakt.js missing coverage on line(s): 5

missing that 0.06% coverage 😲 😉

@marcuspoehls

This comment has been minimized.

Copy link
Contributor Author

marcuspoehls commented Oct 12, 2017

@AdriVanHoudt @ldesplat yeah, I saw the failing tests due to coverage. Can you please tell me how to add test coverage for line 5 in lib/providers/trakt.js?

It's this line:

options = options || {};

in the trakt.js provider.

@AdriVanHoudt

This comment has been minimized.

Copy link
Contributor

AdriVanHoudt commented Oct 12, 2017

So you need a test that covers passing options and one that covers it defaulting to {}

@marcuspoehls

This comment has been minimized.

Copy link
Contributor Author

marcuspoehls commented Oct 12, 2017

Thank you buddy. I didn’t notice at first what was missing. I thought the default option is fine, but —of course— needs testing as well 😄

@marcuspoehls

This comment has been minimized.

Copy link
Contributor Author

marcuspoehls commented Nov 10, 2017

@ldesplat Hi Loïs, I fixed the failing tests and coverage is at 100%. Any chance to get this merged? Thank you!

Copy link
Contributor

AdriVanHoudt left a comment

LGTM

@ldesplat ldesplat added this to the 8.9.0 milestone Nov 19, 2017
@ldesplat

This comment has been minimized.

Copy link
Contributor

ldesplat commented Nov 19, 2017

Failing tests are normal and nothing to worry about. Just new node globals and we're still on such old versions of our dependencies. Thank you!

@ldesplat ldesplat merged commit 03dd779 into hapijs:master Nov 19, 2017
1 check failed
1 check failed
continuous-integration/travis-ci/pr The Travis CI build failed
Details
@marcuspoehls

This comment has been minimized.

Copy link
Contributor Author

marcuspoehls commented Nov 21, 2017

@ldesplat @AdriVanHoudt Thank you for merging!

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.