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

Fix #186 - token and api endpoint not optional #187

Merged
merged 3 commits into from
Feb 1, 2024

Conversation

erichare
Copy link
Collaborator

Note that this applies specifically to the AstraDB (and async version) classes - for the Collection classes, one can provide either an astra_db instance already initialized OR pass a (token + endpoint). So those have been left as is.

@hemidactylus
Copy link
Collaborator

LGTM, just a question: is there any specific reason for replacing the **{...} unpacking of the credentials in the tests with explicit handling -- other than readability/aesthetics? (I mean, were there typing issues or something?)

@erichare
Copy link
Collaborator Author

erichare commented Feb 1, 2024

LGTM, just a question: is there any specific reason for replacing the **{...} unpacking of the credentials in the tests with explicit handling -- other than readability/aesthetics? (I mean, were there typing issues or something?)

Yep, the reason was mypy. It didn’t like the fact that we are unpacking the credentials, because then it’s thinking that the required parameters are actually optional. I tried some kind of hacky workarounds, but in the end thought this was okay. If the universe of optional parameters expands though, we can restore the unpacking syntax for those, and just keep the required ones as explicitly provided.

hopefully that makes sense

@erichare erichare merged commit d7c91f1 into master Feb 1, 2024
2 checks passed
@erichare erichare deleted the bugfix/#186-optional-auth branch February 1, 2024 14:54
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.

Revise Optional[str] constructor parameters when it does not make sense
2 participants