Add an omniauth recipe. #47

Merged
merged 3 commits into from Mar 19, 2013

Conversation

Projects
None yet
2 participants
Member

patriciomacadden commented Mar 19, 2013

This makes the wishlist (#44) a little smaller.

Please feel free to correct me if I missed something (although I think I didn't).

Owner

zzak commented Mar 19, 2013

@patriciomacadden This is good, but I have a couple comments.

The complete example isn't necessary, just add a line below the authentication routes like "Give your app a default route that twitter will redirect back to once everything is all set." Or something like that.

Also, I don't think we should recommend people store their consumer keys and secrets in their actual app, instead let's use ENV and show them how to start the app with the command-line.

Lastly, I think this recipe should be titled: "Twitter Authentication with Omniauth" and the first part should go something like this:

It provides several strategies (released as gems) that provides authentication for a lot of systems, such as Facebook, Google, GitHub, and many more.

Having twitter here is redundant, and I think all the links clutter up the introduction to this great guide.

change the title and the introduction; show how to start the app with…
… environment variables; remove the complete example; add links at the bottom; etc.
Member

patriciomacadden commented Mar 19, 2013

Hey @zzak thanks for the review! I've added a new commit to the PR. I don't want to merge it until you give me your opinion.

Thanks again!

Owner

zzak commented Mar 19, 2013

@patriciomacadden Thanks, looks much better, only one comment

Remove the links section and in the intro use this:

.., such as Facebook, Google, Github, and many more

With that link to their wiki with a full list of strategies.

Then feel free to commit, and thanks again for the help!

Member

patriciomacadden commented Mar 19, 2013

Much better. Merged.

patriciomacadden added a commit that referenced this pull request Mar 19, 2013

@patriciomacadden patriciomacadden merged commit 89ae9a6 into sinatra:master Mar 19, 2013

@patriciomacadden patriciomacadden deleted the patriciomacadden:omniauth branch Mar 19, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment