thoughtbot / carnival

An unobtrusive, developer-friendly way to add comments
MIT License
501 stars 30 forks source link

Allow GitHub authentication without a name defined #283

Closed pbrisbin closed 8 years ago

pbrisbin commented 8 years ago

Use Alternative to grab the profile name from the login key if/when the name key is missing. (This required an orphan Error instance on Text.) Profiles missing a name are possible, while login should always be present[1].

Fixes #282

1: https://github.com/thoughtbot/yesod-auth-oauth2/blob/master/Yesod/Auth/OAuth2/Github.hs#L97

I included a commit that adds stack support. Note that this is not fully-baked; specifically, I can't get stack test or yesod test to work yet, but it does work well enough for me to test-compile the project and run individual spec files. If you'd prefer I keep that commit out, let me know.

/cc @jferris

jferris commented 8 years ago

This looks good to me. I don't object to the Stack changes.

While figuring out the Either/Text/Error changes, it occurred to me that maybe the whole user-to-creds thing should be in the Error monad instead of using Either Text a repeatedly. Thoughts?

pbrisbin commented 8 years ago

Yes, we should definitely be using ExceptT (ErrorT is deprecated in favor of it).

If that were the case, we shouldn't need the orphan Error instance on Text since that constraint is (Functor m, Monad m, Monoid e) => Alternative (ExceptT e m) and Text is a Monoid.

I made the decision to go this way because it was a relatively quick fix for a pretty big bug and elected to save the larger ExceptT discussion for later -- I'm also open to exploring ExceptT now though, as saving it for later would probably result in me not getting to it for some time.

jferris commented 8 years ago

I think going for the smaller fix is cool, particularly since this is a bug in production.

I might take a shot at the ExceptT refactoring on Friday.