-
Notifications
You must be signed in to change notification settings - Fork 283
Fix+docs for background tasks when used by FtResponse #674
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
Conversation
|
Found 1 changed notebook. Review the changes at https://app.gitnotebooks.com/AnswerDotAI/fasthtml/pull/674 |
fasthtml/core.py
Outdated
|
|
||
| def __response__(self, req): | ||
| cts,httphdrs,tasks = _xt_cts(req, self.content) | ||
| tasks = getattr(self, 'background', tasks) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think self.background will always be set, so I'm not sure this is doing anything? Also IIUC it's the wrong way around - we should use the manually added tasks in the response if there, and only then fall back to self.background, shouldn't we?
Either way, it's a good idea to have tests or examples that exercise your code so that you know for sure whether it's working.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the feedback, we'll adjust the API accordingly and see about improving/adding to the tests and docs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got the order of operations corrected, tasks now go for manual before self.background. Thanks @RensDimmendaal for the assist.
ef2afce to
776ded9
Compare
|
Let me know when it's ready for review @pydanny (no hurry though) |
4396c41 to
065039d
Compare
9f5e24d to
017f54f
Compare
Co-Authored-By: Audrey M. Roy Greenfeld <audrey@feldroy.com>
Inspired by Starlette's tests for background tasks Co-Authored-By: Audrey M. Roy Greenfeld <audrey@feldroy.com>
Co-authored-by: Audrey Roy Greenfeld <arg@answer.ai>
Co-authored-by: Audrey Roy Greenfeld <arg@answer.ai>
Co-authored-by: Audrey Roy Greenfeld <arg@answer.ai>
Co-authored-by: Audrey Roy Greenfeld <arg@answer.ai>
Co-authored-by: Audrey Roy Greenfeld <arg@answer.ai>
Co-authored-by: Daniel Roy Greenfeld <drg@answer.ai>
017f54f to
713f067
Compare
|
This is ready for review. |
|
@pydanny This is a really well written tutorial. I love how you use code annotations for the docs with Quarto- extra love. Made it really easy to read and also learn about how it works. |
|
@jph00 do you want to do a review or is this ok to merge? I find this feature to be very valuable in most things that I end up building. |
|
Edit: Q: Can we ship this now? Merging |
name: Pull Request
about: Propose changes to the codebase
title: '[PR] Correct background tasks when used by FtResponse'
labels: 'bug'
assignees: ''
Related Issue
#336 - Background Tasks
Proposed Changes
This PR makes it possible for the FtResponse class to handle Background Tasks
Types of changes
What types of changes does your code introduce? Put an
xin all the boxes that apply:Checklist
Go over all the following points, and put an
xin all the boxes that apply:Additional Information
Any additional information, configuration or data that might be necessary to reproduce the issue.