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

Separate Google into two providers. #228 #235

Merged
merged 1 commit into from Jul 17, 2016

Conversation

@wwalser
Copy link
Contributor

wwalser commented Jun 16, 2016

Issue is at: #228
See #229 for previous discussion.

displayName: profile.name
name: {
given_name: profile.given_name,
family_name: profile.family_name

This comment has been minimized.

Copy link
@qualquervalor

qualquervalor Jun 16, 2016

These should be snake case givenName and familyName

displayName: profile.name,
name: {
given_name: profile.given_name,
family_name: profile.family_name

This comment has been minimized.

Copy link
@qualquervalor

qualquervalor Jun 16, 2016

These should be snake case givenName and familyName

This comment has been minimized.

Copy link
@wwalser

wwalser Jun 16, 2016

Author Contributor

I don't have a problem with following that convention if that's what @ldesplat wants, but that will make this object different to Google Plus's api, which uses underscores.

This is important because anyone upgrading from the previous version of Bell to this one will have to change their code to be snake case as well. Choosing code convention over API compatibility seems like the wrong call IMO.

This comment has been minimized.

Copy link
@ldesplat

ldesplat Jun 19, 2016

Contributor

We unfortunately don't have proper conventions enforced through all the providers so I don't mind having it yet again different if it means a bit less breakage.

@ldesplat ldesplat added this to the 8.0.0 milestone Jun 19, 2016
@@ -20,7 +20,7 @@ const expect = Code.expect;

describe('google', () => {

it('authenticates with mock', { parallel: false }, (done) => {
it('Uses custom profileUrl', { parallel: false }, (done) => {

This comment has been minimized.

Copy link
@ldesplat

ldesplat Jun 19, 2016

Contributor

I guess this needs to be the old description

This comment has been minimized.

Copy link
@wwalser

wwalser Jun 20, 2016

Author Contributor

fixed

@wwalser wwalser force-pushed the wwalser:tale-of-two-googles branch from b77e9f4 to 6143555 Jun 20, 2016
@ldesplat

This comment has been minimized.

Copy link
Contributor

ldesplat commented Jul 16, 2016

Could you rebase this? I am sorry I merged another PR before this one. I'll be ready to make a release as soon as you do!

Thank you for waiting so patiently.

@wwalser

This comment has been minimized.

Copy link
Contributor Author

wwalser commented Jul 16, 2016

I'm out right now. Will get on this when home.

On Sat, 16 Jul 2016, 6:51 AM Loïs Desplat notifications@github.com wrote:

Could you rebase this? I am sorry I merged another PR before this one.
I'll be ready to make a release as soon as you do!

Thank you for waiting so patiently.


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#235 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AADK8q1HGQP4MViWZdKTkUPk49iUpRUUks5qWLe0gaJpZM4I26-s
.

…or the standard Google Identity APIs.
@wwalser wwalser force-pushed the wwalser:tale-of-two-googles branch from 6143555 to 19e37eb Jul 17, 2016
@wwalser

This comment has been minimized.

Copy link
Contributor Author

wwalser commented Jul 17, 2016

@ldesplat Rebased.

@ldesplat

This comment has been minimized.

Copy link
Contributor

ldesplat commented Jul 17, 2016

Thank you so much!

@ldesplat ldesplat merged commit baeb7e5 into hapijs:master Jul 17, 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.