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

Make Auth an abstract interface #1230

Closed
wants to merge 2 commits into from
Closed

Conversation

florimondmanca
Copy link
Member

For consideration, since this is not strictly required and potentially breaking for users that incorrectly relied on Auth() representing "no auth" instead of the base auth class interface, but a potentially nice clarification that Auth should only be treated as an abstract interface that can't be used as-is.

This PR drops the default "do nothing" implementation in Auth, and moves it to a utility NoAuth class.

Prompted since @tomchristie interestingly showed the async-capable Auth class using an abstract .auth_flow() method (raises NotImplementError) in #1217 (comment).

@florimondmanca florimondmanca added the refactor Issues and PRs related to code refactoring label Aug 27, 2020
@florimondmanca florimondmanca changed the title Make Auth an abstract class Make Auth an abstract interface Aug 27, 2020
@tomchristie
Copy link
Member

Thinking about it more, here's an alternative - Keep Auth as a concrete do-nothing implementation, and in our custom auth docs, recommend that:

  • If you're implementing a sync-only auth class, with sync_auth_flow, then you should also implement a stub async_auth_flow that raises a RuntimeError("Cannot use a sync authentication class with AsyncClient").
  • If you're implementing an async-only auth class, with async_auth_flow, then you should also implement a stub sync_auth_flow that raises a RuntimeError("Cannot use an async authentication class with Client").

Benefit there is that we're at least making sure users get clear errors in those cases, rather than a somewhat confusing NotImplmentedError("auth_flow must be overridden")

...?

@florimondmanca
Copy link
Member Author

Yup, sounds good.

@tomchristie
Copy link
Member

Also nice about that is that the docs guidance gets a little simpler...

Either override auth_flow(), or override sync_auth_flow() and async_auth_flow(), (though one of them can just be a raise RuntimeError(...) if you want.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactor Issues and PRs related to code refactoring
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants