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

Serializer Registration #92

Merged
merged 2 commits into from
Mar 13, 2019
Merged

Serializer Registration #92

merged 2 commits into from
Mar 13, 2019

Conversation

scrogson
Copy link
Member

@scrogson scrogson commented Mar 11, 2017

This PR changes the way serializers are registered. It removes the need for serializers to be added to the :oauth2 application config.

Instead, serializers are now registered in code with OAuth2.Client.put_serializer/3. This allows wrapper libraries to provide custom serializers without users needing to be concerned with configuring the :oauth2 application.

/cc @tsubery (#88)

@scrogson scrogson added this to In Progress in dev Mar 11, 2017
@ueberauth ueberauth deleted a comment from coveralls Oct 6, 2017
@coveralls
Copy link

coveralls commented Oct 6, 2017

Coverage Status

Coverage decreased (-0.8%) to 94.231% when pulling 88761bb on register_serializers into 6d1808c on master.

@snewcomer
Copy link

@scrogson Recently came across a good use case here: ueberauth/ueberauth#87.

If you don't have time, I would be happy to pick up the flag on this one. Otherwise, I can review if you are still interested in merging this PR!

@scrogson
Copy link
Member Author

Hey @snewcomer,

I've got a new design that I want to work on that will remove the need for a centralized configuration and rely on the configuration to be on the OAuth2.Client struct itself. This will allow for greater flexibility and power.

@snewcomer
Copy link

This is looking great @scrogson. Lmk if you need an 👁yet!

Signed-off-by: Sonny Scroggin <sonny@scrogg.in>
@scrogson
Copy link
Member Author

@snewcomer @doomspork I'd love some eyes on this! ❤️

@doomspork
Copy link
Member

@scrogson Happy Holidays homie! I will take a peek this weekend and get back to you. Hope all is well with your family 😁

Copy link
Member

@doomspork doomspork left a comment

Choose a reason for hiding this comment

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

Looking good @scrogson! I have only 1 real question and it's the purpose of OAuth2.Serializer.Null. This seems so different from how most Elixir libraries work, I'm not sure it's a win or potential source for confusion. I think you could pull maybe_warn_missing_serializer function out and do away with a serializer that doesn't really serialize.

config/config.exs Show resolved Hide resolved
lib/oauth2/client.ex Outdated Show resolved Hide resolved
lib/oauth2/client.ex Outdated Show resolved Hide resolved
@snewcomer
Copy link

snewcomer commented Jan 5, 2019

Looks great! Not much for me to comment on :)

How does this look (references this PR)? ueberauth/ueberauth_google#58

@scrogson
Copy link
Member Author

How does this look (references this PR)? ueberauth/ueberauth_google#58

@snewcomer hey, sorry for the delay. That PR looks like it will be perfect once this lands. Thanks for that.

I need to clean up a few things and I'll try to get it merged and a new version published.

@scrogson
Copy link
Member Author

@doomspork calling for one last review....

Copy link

@snewcomer snewcomer left a comment

Choose a reason for hiding this comment

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

🙇

Copy link
Member

@doomspork doomspork left a comment

Choose a reason for hiding this comment

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

Looks good @scrogson 👍

@scrogson scrogson merged commit 73ff279 into master Mar 13, 2019
@adamniedzielski
Copy link

@scrogson was it an intentional decision that this PR changes version requirement for hackney from ~> 1.7 to ~> 1.13.0? This is a more restrictive requirement, because ~> 1.7 allows for example 1.15.1, but ~> 1.13.0 no.

I was trying to update oauth2 from 0.9.4 to 1.0.0 and this required a downgrade of hackney (along with some transitive dependencies coming from hackney). At the same time, there's no discussion in this PR related to changing hackney version requirement, so I was thinking that I'd better double-check with you. I can also open a new issue if you prefer 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
dev
In Progress
Development

Successfully merging this pull request may close these issues.

None yet

6 participants