Skip to content

Python SDK: Add read_signals helpers #850

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

Open
wants to merge 10 commits into
base: develop
Choose a base branch
from

Conversation

gazpachoking
Copy link
Contributor

@gazpachoking gazpachoking commented Apr 17, 2025

Adds read_signals functions to all the supported python frameworks. Also adds a dependency for FastAPI so that they can be read idiomatically in that framework. @jmoppel Can you validate the Django one?

I also put a ruff config and formatted the files. Added __all__ to the files as well to make sure that imports that were unused and only included for re-exporting don't get accidentally removed. These can be separated out into different PRs if needed, I was just wanted the ability to easily lint and format my work and ended up adding it in here.

@gazpachoking
Copy link
Contributor Author

Thoughts on the fastapi dependency name? It's SignalsDep right now

@gazpachoking
Copy link
Contributor Author

Don't think the test failures are anything to do with this change..

@bencroker bencroker added the sdk SDK related issues label Apr 29, 2025
@gazpachoking
Copy link
Contributor Author

I changed the FastAPI dep to be called ReadSignals rather than SignalsDep. I think it's a bit better.

@gazpachoking
Copy link
Contributor Author

gazpachoking commented May 6, 2025

I tested the django read_signals helper now, and changed the behavior to return None rather than error if the request was not sent from datastar, or had no signals attached. This lets FastAPI and Litestar use read_signals as a dependency, even if the route handler gets hit by requests that aren't from datastar. I think this is in a pretty good state now.

Also added examples of how to use read_signals for all the different frameworks.

@bencroker
Copy link
Collaborator

I changed the FastAPI dep to be called ReadSignals rather than SignalsDep. I think it's a bit better.

Naming conventions should follow the SDK spec as closely as possible, so it should absolutely be ReadSignals or read_signals, as appropriate.
https://github.com/starfederation/datastar/tree/develop/sdk#readsignalsr-httprequest-signals-any-error

Factor out a common implementation for all the frameworks.
Make all the example scripts directly runnable.
@gazpachoking
Copy link
Contributor Author

Previous implementation raised an exception with the excludeSignals option coming up in the next RC. Fixed that issue, and factored out the logic to one place so that it's not implemented separately for each framework.

@bencroker
Copy link
Collaborator

This is looking great! If we could get a Python SKD contributor to look at and approve this PR, that would be reassuring, as I’m less familiar with Python.

@gazpachoking
Copy link
Contributor Author

In hopes of making reviewing this easier I stripped out all the other fixes and cleanups not related directly to read_signals. I still think those are good, but I'll raise them in a separate PR.

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

Successfully merging this pull request may close these issues.

2 participants