Skip to content

Add Rails setup to Readme#50

Closed
testbrian wants to merge 1 commit intoomniauth:masterfrom
testbrian:patch-2
Closed

Add Rails setup to Readme#50
testbrian wants to merge 1 commit intoomniauth:masterfrom
testbrian:patch-2

Conversation

@testbrian
Copy link
Copy Markdown
Contributor

Changed ENV['GITHUB_KEY'], ENV['GITHUB_SECRET'] to ENV['GITHUB_CLIENT_ID'], ENV['GITHUB_CLIENT_SECRET'] to match Github credentials and added a little info.

Comment thread README.md Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Add a blank line above and 4 space indent this to make MD format it as code?

gem 'omniauth-github'

@jasonnoble
Copy link
Copy Markdown
Contributor

Maybe we should add deprecation notices for ENV['GITHUB_KEY'], ENV['GITHUB_SECRET']?

@testbrian
Copy link
Copy Markdown
Contributor Author

There isn't a deprecation AFAIK, just a slight rename so a deprecation warning wouldn't be necessary.

@matkoniecz
Copy link
Copy Markdown

@testbrian Thanks, this guide is very helpful. It is unfortunate that this PR was not accepted as it is currently a bit hard to find.

@testbrian testbrian force-pushed the patch-2 branch 2 times, most recently from b497860 to 9c90d0b Compare October 27, 2016 11:57
@testbrian
Copy link
Copy Markdown
Contributor Author

How about merging then?

@tmilewski
Copy link
Copy Markdown
Member

@matkoniecz @testbrian Hey guys! I just got added to the repo and I'm going through old PRs.
Thank you for this!

Considering that Omniauth and this gem, in particular, isn't specific to Rails, I'd be hesitant to add this entire setup to the README. With this said it'd work perfectly as a Wiki article which we can link to in the README.

I've set up a page here: https://github.com/intridea/omniauth-github/wiki/Rails-Usage

What do you think?

@tmilewski
Copy link
Copy Markdown
Member

Moving to close.

@tmilewski tmilewski closed this Feb 12, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants