🐛 Fix return value of #add_provider_to_user on User instance#224
🐛 Fix return value of #add_provider_to_user on User instance#224mtomov wants to merge 1 commit intoSorcery:mainfrom
Conversation
mladenilic
left a comment
There was a problem hiding this comment.
Hi @mtomov , thanks for the PR.
Can you please link the example you are referring to? As you said, it makes more sense to return the authentication instance.
3aefbaa to
ac9fd85
Compare
|
Thanks @mladenilic for looking over. I guess indeed you're right that there's only one place referring to the return value of the method being user instead of authentication: sorcery/spec/rails_app/app/controllers/sorcery_controller.rb Lines 448 to 452 in c30cefa Happy to change the code back to return an authentication, adjust the tests, and change that example code line, etc.. if you think that's more sensible. |
|
It seems that the original intention was to return the user instance. However, I agree with you that returning authentication instance makes more sense. I think we should revert back to returning authentication instance, but keep everything else (guard clause simplification and Thanks! |
* `#add_provider_to_user` is returning an Authentication instance
using a `user` variable name.
* This commit changes the variable name to match the return type, i.e.
`user` -> `authentication`
* Similarly amend a confusing test example
ac9fd85 to
48eeee9
Compare
|
Thanks for your feedback @mladenilic . All amended as talked about. Let me know if you'd like me to change anything else. Will be happy to. Thanks! |
Hi, thank you for the great library! Using it with ease. Just noticed a small bug on the return value of
#add_provider_to_userhere:https://github.com/Sorcery/sorcery/blob/master/lib/sorcery/model/submodules/external.rb#L99-L100
Before, the method would return an instance of Authentication,
instead of the original intention to return an instance of the
current user.
Arguably returning the the instance of the current user is not very useful,
as it's already known 🤷♂, but at least it matches the current
example here:
sorcery/spec/rails_app/app/controllers/sorcery_controller.rb
Lines 447 to 452 in c30cefa