-
-
Notifications
You must be signed in to change notification settings - Fork 974
feat(request): add last_event_id property #2587
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
base: master
Are you sure you want to change the base?
Conversation
Introduce req.last_event_id to provide convenient access to the Last-Event-ID header. Closes falconry#2580.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #2587 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 64 64
Lines 7866 7869 +3
Branches 1078 1078
=========================================
+ Hits 7866 7869 +3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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 once again for the PR, it looks good for most of it.
I've requested a couple of improvements inline.
Another thing, it seems like the PR summary is heavily influenced by "AI", if not outright churned by Cursor or similar... 👿 Don't do this! It is not the end of the world, but it would be much more useful if you wrote a sentence or two yourself.
This is verbose and prone to typos (e.g., casing issues).
No, the get_header(...) name argument is not case sensitive!
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.
Could you rewrite this more as a story (what has been done), not in imperative (as docstrings)?
falcon/request.py
Outdated
| str: The value of the Last-Event-ID header, or None if the | ||
| header was not present in the request. | ||
| """ | ||
| return self.get_header('Last-Event-ID') |
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.
Could we optimize this by operating on the internal headers directly?
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 updated the PR summary and optimized the implementation to use _header_property in both WSGI and ASGI files as requested. I also updated the newsfragment to follow the narrative style. Ready for re-review!
Introduce req.last_event_id to provide convenient access to the Last-Event-ID header. Closes #2580.
Summary of Changes
I added
req.last_event_idto theRequestclass to make it easier to access theLast-Event-IDheader, which is standard for Server-Sent Events. This implementation uses_header_propertyhelper in both WSGI and ASGI files for performance and consistency. I've also included the necessary tests and a newsfragment.Related Issues
Closes #2580
Pull Request Checklist
This is just a reminder about the most common mistakes. Please make sure that you tick all appropriate boxes. Reading our contribution guide at least once will save you a few review cycles!
If an item doesn't apply to your pull request, check it anyway to make it apparent that there's nothing to do.
tox.docs/.docs/.versionadded,versionchanged, ordeprecateddirectives.docs/_newsfragments/, with the file name format{issue_number}.{fragment_type}.rst. (Runtox -e towncrier, and inspectdocs/_build/html/changes/in the browser to ensure it renders correctly.)