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

Reworked how the decoder argument works #156

Closed
wants to merge 3 commits into
base: master
from

Conversation

Projects
None yet
2 participants
@alertedsnake

alertedsnake commented Apr 25, 2014

The decoder argument is actually required on a few other methods but wasn't there, specifically what happens when you need a decoder but you call service.get_auth_session() and it defaults to not passing a decoder to session.get_access_token()? It breaks.

Yes, you can replace the call to get_auth_session() with get_access_token() and get_session(), but why have the shortcut method if it doesn't work in all cases?

So what I've done here is added a parameter to the Session object itself, so you can set it once and forget it for good. I thought about adding it just to get_auth_session() but do we really want to be passing methods arguments then they could just as easily be set on the class?

Reworked how the decoder argument works - it's actually required on a…
… few other

methods but wasn't there, specifically what happens when you need a decoder but
you call service.get_auth_session() and it defaults to not passing a decoder to
session.get_access_token()?  It breaks.

Yes, you can replace the call to get_auth_session() with get_access_token() and
get_session(), but why have the shortcut method if it doesn't work in all
cases?

So what I've done here is added a parameter to the Session object itself, so
you can set it once and forget it for good.
@maxcountryman

View changes

Show outdated Hide outdated rauth/service.py
@maxcountryman

This comment has been minimized.

Show comment
Hide comment
@maxcountryman

maxcountryman Apr 25, 2014

Contributor

Looks good overall. You might also want to add a simple unit test that shows the default decoder is the fn you expect and that passing in a custom decoder does what you expect. Also would be nice to see this mentioned in the docstring so the documentation is updated as well.

Contributor

maxcountryman commented Apr 25, 2014

Looks good overall. You might also want to add a simple unit test that shows the default decoder is the fn you expect and that passing in a custom decoder does what you expect. Also would be nice to see this mentioned in the docstring so the documentation is updated as well.

@alertedsnake

This comment has been minimized.

Show comment
Hide comment
@alertedsnake

alertedsnake Apr 25, 2014

If we're going to get specific on the comparison, then perhaps the test
should be "if callable(decoder)" because we don't actually care if it's not
None, we care if it's a useful callable. In most cases the simple boolean
test is going to be fine.

Also it turns out I was doing something wrong when I decided I needed this
feature, so while I agree that unit tests and documentation would be
helpful, and that I'll add them, it might be a little while before I get to
it, the library works for me without this patch right now :)

On Fri, Apr 25, 2014 at 1:20 PM, Max Countryman notifications@github.comwrote:

Looks good overall. You might also want to add a simple unit test that
shows the default decoder is the fn you expect and that passing in a custom
decoder does what you expect. Also would be nice to see this mentioned in
the docstring so the documentation is updated as well.


Reply to this email directly or view it on GitHubhttps://github.com/litl/rauth/pull/156#issuecomment-41417554
.

Michael Stella

alertedsnake commented Apr 25, 2014

If we're going to get specific on the comparison, then perhaps the test
should be "if callable(decoder)" because we don't actually care if it's not
None, we care if it's a useful callable. In most cases the simple boolean
test is going to be fine.

Also it turns out I was doing something wrong when I decided I needed this
feature, so while I agree that unit tests and documentation would be
helpful, and that I'll add them, it might be a little while before I get to
it, the library works for me without this patch right now :)

On Fri, Apr 25, 2014 at 1:20 PM, Max Countryman notifications@github.comwrote:

Looks good overall. You might also want to add a simple unit test that
shows the default decoder is the fn you expect and that passing in a custom
decoder does what you expect. Also would be nice to see this mentioned in
the docstring so the documentation is updated as well.


Reply to this email directly or view it on GitHubhttps://github.com/litl/rauth/pull/156#issuecomment-41417554
.

Michael Stella

@maxcountryman

This comment has been minimized.

Show comment
Hide comment
@maxcountryman

maxcountryman Apr 25, 2014

Contributor

the library works for me without this patch right now :)

This is always the best case. :)

Contributor

maxcountryman commented Apr 25, 2014

the library works for me without this patch right now :)

This is always the best case. :)

alertedsnake and others added some commits Apr 29, 2014

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