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

Initial work to support streams #155

Merged
merged 20 commits into from
Mar 14, 2024
Merged

Initial work to support streams #155

merged 20 commits into from
Mar 14, 2024

Conversation

marrony
Copy link
Contributor

@marrony marrony commented Feb 29, 2024

ENG-6011, ENG-6012, ENG-6013, ENG-6014, ENG-6039

With this PR we are already able to support the following snippet of code:

from fauna import fql
from fauna.client import Client

client = Client(endpoint = "http://localhost:8443", secret = "secret")

token = client.query(fql('Products.all().toStream()'))

with client.stream(token.data) as stream:
  for evt in stream:
    print(evt)

    if some_condition:
      stream.close()

@marrony marrony marked this pull request as draft February 29, 2024 21:47
@marrony marrony force-pushed the stream branch 3 times, most recently from 2e780fc to 8d00cd4 Compare February 29, 2024 21:58
@marrony marrony marked this pull request as ready for review February 29, 2024 22:02
Copy link
Contributor

@fauna-chase fauna-chase left a comment

Choose a reason for hiding this comment

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

General API use looks correct, I have a question on error handling in the pr.

Also thinking through the restarts a bit, we are going to need to track the last seen transaction ts for each stream the client has open, are we setup to do that as is? We won't want to use the general client transaction ts.

Do we do any sort of integration tests against an actual running fauna in this driver? Would be good to have some actual streaming tests in place and also would make it fairly straight forward to validate the error ux.

fauna/client/client.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@pnwpedro pnwpedro left a comment

Choose a reason for hiding this comment

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

One comment about avoiding httpx as a top level import in the client.

Also, let's add integration tests. I'd prefer those over unit tests in general.

fauna/client/client.py Outdated Show resolved Hide resolved
@pnwpedro
Copy link
Collaborator

pnwpedro commented Mar 5, 2024

Also, this fails against 3.9 because of

E   TypeError: unsupported operand type(s) for |: 'type' and 'type'

fauna/client/client.py Outdated Show resolved Hide resolved
fauna/client/client.py Outdated Show resolved Hide resolved
fauna/client/client.py Outdated Show resolved Hide resolved
tests/unit/test_client.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@pnwpedro pnwpedro left a comment

Choose a reason for hiding this comment

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

Looking pretty good! A few more items I noticed. Let me know if you want to discuss any

fauna/client/client.py Outdated Show resolved Hide resolved
fauna/client/client.py Outdated Show resolved Hide resolved
fauna/client/client.py Outdated Show resolved Hide resolved
fauna/client/client.py Outdated Show resolved Hide resolved
@marrony marrony force-pushed the stream branch 19 times, most recently from c32b2a9 to 5077808 Compare March 12, 2024 18:17
Copy link
Collaborator

@pnwpedro pnwpedro left a comment

Choose a reason for hiding this comment

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

lgtm!

@marrony marrony merged commit 6d0d365 into main Mar 14, 2024
6 checks passed
@marrony marrony deleted the stream branch March 14, 2024 18:59
pnwpedro added a commit that referenced this pull request Apr 1, 2024
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.

4 participants