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

Extract handle_missing_response! #25

Merged
merged 2 commits into from
Aug 10, 2016
Merged

Extract handle_missing_response! #25

merged 2 commits into from
Aug 10, 2016

Conversation

ecoologic
Copy link
Contributor

  • Fix build badge (by removing EOL ruby version)
  • Extract method to keep consistent level of abstraction
  • Add an extra debug log (that pairs with the one in handle_response)

Happy to rebase it or break it down in multiple prs if you like.

def handle_missing_response!
logger.debug("Delegate authentication to Rack::OpenID by throwing a 401")

opts = { identifier: identity_url, return_to: return_url, trust_root: trust_root, method: 'post' }
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have taken the liberty of converting this hash to 1.9 syntax

@nbudin
Copy link
Owner

nbudin commented Aug 10, 2016

Looks good, thanks for the PR!

@nbudin nbudin merged commit f3bc751 into nbudin:master Aug 10, 2016
@ecoologic
Copy link
Contributor Author

The merge was really quick!, @nbudin thank you for the gem!

@nbudin
Copy link
Owner

nbudin commented Aug 10, 2016

Sure thing @ecoologic! Do you want me to make a release with this change as well? (I'm not sure what functionality it is enabling, it just seems like a nice code cleanup - but if you have projects depending on this change being released I'll be happy to cut a new version.)

@ecoologic
Copy link
Contributor Author

Right now I'm just studying the gem to use it in a rails prj. Maybe officially dropping 1.9 is a CHANGELOG worthy note.

This PR is plain refactoring (except for the added debug log), it doesn't seem very urgent to me, so, this is not affecting me, I leave that to you.

@nbudin
Copy link
Owner

nbudin commented Aug 10, 2016

OK, cool, in that case I will leave the 1.9 drop for the next feature release. Thanks again for the cleanup!

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.

None yet

2 participants