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

Remove UJSONResponse #1047

Merged
merged 4 commits into from
Nov 8, 2020
Merged

Remove UJSONResponse #1047

merged 4 commits into from
Nov 8, 2020

Conversation

JayH5
Copy link
Member

@JayH5 JayH5 commented Sep 6, 2020

Closes #901

It's easy enough to implement your own UJSONResponse response class if you need it:

from typing import Any

import ujson
from starlette.responses import JSONResponse

class UJSONResponse(JSONResponse):
    def render(self, content: Any) -> bytes:
        return ujson.dumps(content, ensure_ascii=False).encode("utf-8")

@euri10
Copy link
Member

euri10 commented Sep 6, 2020

I'm all in for that removal !
Not a blocker on my side but could we add the example you provided (or even better with orjson ) in docs/responses.md ?

@JayH5 JayH5 added the clean up Refinement to existing functionality label Sep 11, 2020
@JayH5
Copy link
Member Author

JayH5 commented Sep 12, 2020

@euri10 I wrote a bit about custom JSON serialization in responses.md. lmk what you think!

@euri10
Copy link
Member

euri10 commented Sep 12, 2020

@euri10 I wrote a bit about custom JSON serialization in responses.md. lmk what you think!

looks perfect ! 👍

@tomchristie
Copy link
Member

Fab yup!
Will need calling out in release notes and a median version bump.
Great stuff, thank you!

@erewok
Copy link
Contributor

erewok commented Nov 8, 2020

@JayH5, this would have been a good one to include in the 0.14.0 release. Can we merge it and get it released after the fact as 0.14.1? It's pretty close to the "breaking-change" semver bump and with the recent release of 0.14.0 most users have probably not upgraded yet?

@JayH5
Copy link
Member Author

JayH5 commented Nov 8, 2020

@erewok yeah sorry, I didn't want it to get in the way of all the fixes in "0.13.9" and then when we switched to 0.14 I forgot about it.

Happy to squeeze it in if you want to do that? I won't be able to organise a release right now (haven't actually done one before), but if you want to merge this and do one be my guest.

@erewok
Copy link
Contributor

erewok commented Nov 8, 2020

@JayH5, no worries!

I do think we could take advantage of the "breaking change" version bump to merge this (and probably any other test or documentation changes that are easily to include) and make another release.

Would you be up for merging this and issuing a release? (For reference: https://www.python-httpx.org/contributing/#releasing)

@JayH5
Copy link
Member Author

JayH5 commented Nov 8, 2020

(and probably any other test or documentation changes that are easily to include)

I don't see any other obvious candidate PRs. Will give this a go.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clean up Refinement to existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Consider removing UJSONResponse from core
4 participants