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

Clean up authentication #93

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Lordshinjo
Copy link
Member

  • Remove unused methods set_client_session, setup, handle_err/error
  • Add type hints whenever possible
  • Remove requests.auth import try/except in BasicAuthentication as no exception could ever happen due to it already being imported
  • Add proper type hints for auth in client and constants

- Remove unused methods set_client_session, setup, handle_err/error
- Add type hints whenever possible
- Remove requests.auth import try/except in BasicAuthentication as no
exception could ever happen due to it already being imported
- Add proper type hints for auth in client and constants
@cla-bot cla-bot bot added the cla-signed label Aug 5, 2021
@ebyhr
Copy link
Member

ebyhr commented Aug 5, 2021

Remove unused methods set_client_session, setup, handle_err/error

I'm not sure whether it's "unused" by users. If users have below code, this commit will be breaking change.

auth = trino.auth.BasicAuthentication("principal id", "password")
auth.set_client_session(...)

Copy link
Member

@hashhar hashhar left a comment

Choose a reason for hiding this comment

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

Please extract type annotations into separate commit.

As for removing set_client_session I think it's a breaking change and it's useful to allow users to pass in their own sessions to handle authentication mechanisms that aren't implemented in the client yet.

@Lordshinjo
Copy link
Member Author

What about setup and handle_err / handle_error, are these ok to remove?
I highly suspect that's a yes, but just for clarity.

Please extract type annotations into separate commit.

Do you mean a separate PR?
(Since I think you rebase and squash PRs, so a separate commit wouldn't be visible in the history.)

@hashhar
Copy link
Member

hashhar commented Aug 5, 2021

We don't "squash" generally. Only "rebase and merge". Separate PR would be better though - we can get it merged earlier since type-annotations are not a controversial change.

setup seems to be used so I'm not sure you can remove it. handle_err seems to be a typo - it should've been handle_error I guess. I'd leave it in place though since there's no harm in keeping them and they might be a breaking change at this point.

@Lordshinjo
Copy link
Member Author

setup is definitely not used as the method is broken (the properties accessed are the wrong ones, and the method never matches the base class' signature).
If we leave it in, it might make more sense to actually fix it and call it in the constructor of TrinoRequest (instead of only calling set_http_session).

handle_err / handle_error is a bit weird to me as even ignoring the typo, it is never called nor documented in trino-python-client, and the implementation is only ever pass, which doesn't return a value suggesting that something could be handled in any meaningful way.

And I can't really only extract the type annotations in a separate PR, since fixing the types requires at least fixing setup.

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

Successfully merging this pull request may close these issues.

3 participants