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 authenticators accept Sequence #322

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

Conversation

martinHrabina
Copy link

Addresses issue #320, allows authenticator parameter to accept Sequence instead of just List and additionally does not ignore unsupported type but raises an error.

Copy link
Contributor

@nlawrence22 nlawrence22 left a comment

Choose a reason for hiding this comment

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

Overall looks good, just one question I need to understand before approving. 👍

if isinstance(authenticators, list):
authenticators_list = authenticators
if isinstance(authenticators, Sequence):
authenticators_list = list(authenticators)
Copy link
Contributor

Choose a reason for hiding this comment

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

We're explicitly casting to a list here because we expect a list where this value is used?

Copy link
Author

@martinHrabina martinHrabina Dec 12, 2024

Choose a reason for hiding this comment

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

Honestly I am not really sure, in versions 3.1.1 and older, tuple was okay, but since 3.2.0 the check isinstance(authenticators, list) appeared for some reason. So I am casting to list here just to be sure (unsure if list is expected somewhere along the way or not).
An alternative approach here would be to revert the original change (link to diff) and use the value unchanged if it does not match either a list, None or an Authenticator type.

Comment on lines +557 to +560
raise TypeError(
f"Unexpected data type '{type(authenticators)}' for parameter 'authenticators'. "
f"Expected type is either Authenticator, Sequence or None."
)
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@nlawrence22
Copy link
Contributor

Also, thank you for the contribution! Sorry, at first I thought this had come from my teammate -- we appreciate you taking the time to improve flask-rebar!

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

Successfully merging this pull request may close these issues.

3 participants