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

docs(recipes): add text/plain media handler recipe #2419

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

EricGoulart
Copy link
Contributor

Summary of Changes

introduce a new recipe demonstrating a text/plain media handler implementation the recipe includes serialization and deserialization logic, with support for custom charsets.

Related Issues

Closes #2037

Copy link
Member

@vytas7 vytas7 left a comment

Choose a reason for hiding this comment

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

Thanks for this pull request, @EricGoulart @diegomirandap!

However, it seems that you have only added the Python file, and tests (that is nice 👍); you also need to write the recipe itself (place it under docs/user/recipes/), and include your file.

Moreover, it seems that you need to address merge conflicts, as you based your work and older version of our main branch.

(See other comments inline.)

@classmethod
@functools.lru_cache
def _get_charset(cls, content_type):
_, params = cgi.parse_header(content_type)
Copy link
Member

Choose a reason for hiding this comment

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

cgi has been removed from CPython 3.13+. You can use our own replacement: falcon.parse_header().


assert response.status_code == 200
assert response.content_type == 'text/plain'
assert response.text == payload
Copy link
Member

Choose a reason for hiding this comment

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

Newline missing at the end of file.

Copy link

codecov bot commented Dec 9, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (11076b8) to head (ca349ed).

Additional details and impacted files
@@            Coverage Diff            @@
##            master     #2419   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           64        64           
  Lines         7728      7728           
  Branches      1071      1071           
=========================================
  Hits          7728      7728           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@vytas7 vytas7 left a comment

Choose a reason for hiding this comment

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

Looking better now, thanks!
Could you make sure tox passes locally first? I.e., run ruff to reformat, etc.

@vytas7 vytas7 changed the title feat(recipes): add text/plain media handler recipe docs(recipes): add text/plain media handler recipe Dec 11, 2024
@EricGoulart EricGoulart requested a review from vytas7 December 11, 2024 19:22
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.

Media handler for text/plain
3 participants