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

feat: support "+json"-suffixed response media types #3096

Merged
merged 7 commits into from Feb 13, 2024

Conversation

bunny-therapist
Copy link

Giving route handlers a response media type of the form "application/+json" automatically uses json encoding for the response.

Closes #3088

@bunny-therapist bunny-therapist requested review from a team as code owners February 12, 2024 11:14
Copy link

codecov bot commented Feb 12, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (bfeb5e9) 98.23% compared to head (e0f34e4) 98.25%.
Report is 48 commits behind head on develop.

❗ Current head e0f34e4 differs from pull request most recent head 4247e47. Consider uploading reports for the commit 4247e47 to get more accurate results

Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #3096      +/-   ##
===========================================
+ Coverage    98.23%   98.25%   +0.02%     
===========================================
  Files          312      320       +8     
  Lines        14121    14363     +242     
  Branches      2430     2307     -123     
===========================================
+ Hits         13872    14113     +241     
- Misses         107      109       +2     
+ Partials       142      141       -1     

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

@bunny-therapist
Copy link
Author

The documentation preview does not have my changes. Am I supposed to build it manually somehow?

@provinzkraut
Copy link
Member

The documentation preview does not have my changes. Am I supposed to build it manually somehow?

Seems to be there?

Screenshot from 2024-02-12 14-13-26

https://litestar-org.github.io/litestar-docs-preview/3096/usage/responses.html#json-responses

@bunny-therapist
Copy link
Author

"build docs" failed for the commit with

/home/runner/work/litestar/litestar/docs/reference/cli.rst:7: ERROR: "<class 'litestar.cli._utils.LitestarExtensionGroup'>" of type "litestar.cli:litestar_group" is not click.Command or click.Group."click.BaseCommand"

Sphinx parallel build error:
tools.sphinx_ext.run_examples.StartupError: App docs/examples/data_transfer_objects/factory/providing_values_for_nested_data.py failed to come online
make: *** [Makefile:152: docs] Error 2
reading sources... [ 89%] usage/databases/piccolo .. usage/metrics/prometheus
Error: Process completed with exit code 2.

I can't see how I could have caused this or what I can do about it?

@provinzkraut
Copy link
Member

"build docs" failed for the commit with

/home/runner/work/litestar/litestar/docs/reference/cli.rst:7: ERROR: "<class 'litestar.cli._utils.LitestarExtensionGroup'>" of type "litestar.cli:litestar_group" is not click.Command or click.Group."click.BaseCommand"

Sphinx parallel build error:
tools.sphinx_ext.run_examples.StartupError: App docs/examples/data_transfer_objects/factory/providing_values_for_nested_data.py failed to come online
make: *** [Makefile:152: docs] Error 2
reading sources... [ 89%] usage/databases/piccolo .. usage/metrics/prometheus
Error: Process completed with exit code 2.

I can't see how I could have caused this or what I can do about it?

This was fixed in main already iirc. Should be good now here too.

@bunny-therapist
Copy link
Author

"build docs" failed for the commit with

/home/runner/work/litestar/litestar/docs/reference/cli.rst:7: ERROR: "<class 'litestar.cli._utils.LitestarExtensionGroup'>" of type "litestar.cli:litestar_group" is not click.Command or click.Group."click.BaseCommand"

Sphinx parallel build error:
tools.sphinx_ext.run_examples.StartupError: App docs/examples/data_transfer_objects/factory/providing_values_for_nested_data.py failed to come online
make: *** [Makefile:152: docs] Error 2
reading sources... [ 89%] usage/databases/piccolo .. usage/metrics/prometheus
Error: Process completed with exit code 2.

I can't see how I could have caused this or what I can do about it?

This was fixed in main already iirc. Should be good now here too.

But this is not based on main, but on develop, which is 9 commits behind main.

@provinzkraut
Copy link
Member

But this is not based on main, but on develop, which is 9 commits behind main.

Yeah, that was the issue. develop wasn't up to date. The fix was to update develop and subsequently your branch so they had the latest changes from main. Sorry if my initial explanation was a bit vague 👀

@bunny-therapist
Copy link
Author

Some checks failed with docker compose errors now?

Copy link
Member

@provinzkraut provinzkraut left a comment

Choose a reason for hiding this comment

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

I've got one comment about the example, other than that this is good to go in!

avikstroem added 2 commits February 13, 2024 14:02
@provinzkraut provinzkraut enabled auto-merge (squash) February 13, 2024 13:23
@bunny-therapist
Copy link
Author

A test failed mysteriously, but I think it is marked as "flaky".

@bunny-therapist
Copy link
Author

Huh. I guess dict[str, Any] only became a thing in python 3.9. I will update.

Using built-in types as generics only
became possible in 3.9
auto-merge was automatically disabled February 13, 2024 13:29

Head branch was pushed to by a user without write access

@bunny-therapist
Copy link
Author

All checks are passing now. I am not sure what "update branch" strategy is preferred (merge vs rebase) so I will leave that to you.

@provinzkraut
Copy link
Member

All checks are passing now. I am not sure what "update branch" strategy is preferred (merge vs rebase) so I will leave that to you.

Do whatever you prefer. Personally, I like rebase more, but it doesn't really make a difference here since we squash merge PRs anyway.

@provinzkraut provinzkraut enabled auto-merge (squash) February 13, 2024 14:21
@provinzkraut provinzkraut merged commit db96d94 into litestar-org:develop Feb 13, 2024
17 checks passed
Copy link

Documentation preview will be available shortly at https://litestar-org.github.io/litestar-docs-preview/3096

@bunny-therapist
Copy link
Author

Should I have been added to some "all contributors" file? I see that happening in other PRs but I don't know how this works.

@provinzkraut
Copy link
Member

Yes, you should have!

You can just request that yourself by making a comment like this:

@all-contributors add @bunny-therapist for code.

Copy link
Contributor

@provinzkraut

I've put up a pull request to add @bunny-therapist! 🎉

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.

Enhancement: Support "+json" suffixed media types per RFC 6839
2 participants