10000 Fix subscriber decorator bug by pdeziel · Pull Request #81 · rotationalio/pyensign · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Fix subscriber decorator bug #81

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Oct 20, 2023
Merged

Fix subscriber decorator bug #81

merged 2 commits into from
Oct 20, 2023

Conversation

pdeziel
Copy link
Collaborator
@pdeziel pdeziel commented Oct 18, 2023

This PR fixes a bug where decorator chaining was not working, e.g. it should be possible to make a function both a subscriber and publisher:

@authenticate(cred_path=ENSIGN_CREDS_PATH)
@subscriber("flight-positions")
@publisher("text-updates")
async def get_updates(updates):
    async for event in updates:
        flight = json.loads(event.data)
        text = humanize(flight)
        yield text

Previously this raised a TypeError so I added a test for this and fixed the bug.

  1. Fix subscriber decorator bug
  2. Update subscribe and query docstrings for clarity

TODOs and questions

Still to do:

  • [ ]
  • [ ]
  • [ ]

Questions for the @rotationalio/maintainers:

  • [ ]
  • [ ]

CHECKLIST

  • Is the commit message formatted correctly?
  • Do all of your functions and methods have docstrings?
  • Have you added/updated unit tests where appropriate?
  • Have you run the unit tests using pytest?
  • Is your code style correct (are you using PEP8, pyflakes)?

@shortcut-integration
Copy link

This pull request has been linked to Shortcut Story #21922: TypeError trying to chain decorators.

pdeziel
pdeziel commented Oct 18, 2023
View reviewed changes
pdeziel
pdeziel commented Oct 18, 2023
View reviewed changes
@pdeziel pdeziel requested a review from rebeccabilbro October 18, 2023 16:09
bbengfort
bbengfort approved these changes Oct 20, 2023
View reviewed changes
Copy link
Contributor
@bbengfort bbengfort left a comment

Choose a reason for hiding this comment

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

Looks good - thank you for clarifying the docstring and making the fix to the aysnc generator.

pyensign/ensign.py
Comment on lines +223 to +226
Subscribe to realtime events from a set of Ensign topics. This method returns
an async generator that yields Event objects, so the `async for` syntax can be
used to process events as they are received. To retrieve historical events, use
the `query()` method instead.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Subscribe to realtime events from a set of Ensign topics. This method returns
an async generator that yields Event objects, so the `async for` syntax can be
used to process events as they are received. To retrieve historical events, use
the `query()` method instead.
Subscribe to realtime events from a set of Ensign topics. This method returns
an async generator that yields Event objects, so the `async for` syntax can be
used to process events as they are published. To retrieve events published
before now, use the `query()` method instead.

pyensign/ensign.py
Comment on lines +306 to +309
8000
Execute an EnSQL query to retrieve historical events. This method returns a
cursor that can be used to fetch the results of the query, via `fetchone()`,
`fetchmany()`, or `fetchall()` methods. Alternatively, `async for` syntax can
be used to iterate over the results.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Execute an EnSQL query to retrieve historical events. This method returns a
cursor that can be used to fetch the results of the query, via `fetchone()`,
`fetchmany()`, or `fetchall()` methods. Alternatively, `async for` syntax can
be used to iterate over the results.
Execute an EnSQL query to retrieve events events from the topic no matter
when they were published. This method returns a cursor that can be used to
fetch the results of the query, via `fetchone()`, `fetchmany()`, or `fetchall()`
methods. Alternatively, `async for` syntax can be used to iterate over the
results.

@bbengfort bbengfort merged commit 8e61ed8 into develop Oct 20, 2023
@bbengfort bbengfort deleted the sc-21922 branch October 20, 2023 21:24
@bbengfort bbengfort restored the sc-21922 branch October 20, 2023 21:24
@bbengfort bbengfort deleted the sc-21922 branch October 20, 2023 21:24
@bbengfort
Copy link
Contributor

Well … I just merged the PR without merging the suggestions 🤦🏽 that was silly of me, sorry about that. If you like the suggestions, feel free to open a new PR and merge them in, otherwise don't worry about it.

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

@bbengfort bbengfort bbengfort approved these changes

@rebeccabilbro rebeccabilbro Awaiting requested review from rebeccabilbro

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@pdeziel @bbengfort
0