Skip to content

Conversation

@flyingsilverfin
Copy link
Member

No description provided.

@whummer whummer self-requested a review October 22, 2025 13:28
Copy link
Collaborator

@whummer whummer left a comment

Choose a reason for hiding this comment

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

Looks awesome - great set of changes @flyingsilverfin ! 🚀

I checked out the branch locally and deployed the app - worked smoothly. 👌

image

Only added a minor comment - perhaps we could make the driver configurable? (but could also be tackled in a future iteration.. 👍 )

from typedb.driver import (
TypeDB,
from typedb_http_driver import (
TypeDBHttpDriver,
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: would be great if we could make the driver configurable, to make it easy to switch between different implementations (e.g., via an environment variable configured for the Lambda function).

Shouldn't be a blocker here, but would be a nice addition to simplify testing different drivers.. 👍

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah to be honest I've been wondering why i can't reproduce this in any other way. I wonder if it's something with Docker not doing so well with HTTP streaming or something! I don't quite know yet, but will definitely be digging into this more. If i can get it ironed out i'd take out the HTTP driver to be honest -- not as elegant/easy to use!


def _get_token(self) -> str:
"""Get authentication token, refreshing if necessary"""
if self.token:
Copy link
Member Author

Choose a reason for hiding this comment

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

also im not sure token is ever unset even if it expires 🤔

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