Skip to content

Replace ujson with orjson - #29

Open
valrus wants to merge 6 commits into
python-lsp:developfrom
valrus:replace-ujson-with-orjson
Open

valrus wants to merge 6 commits into
python-lsp:developfrom
valrus:replace-ujson-with-orjson

Conversation

@valrus

@valrus valrus commented Aug 11, 2024

Copy link
Copy Markdown

This PR is an attempt to carry #28 to completion, making the necessary changes to ensure compatibility with orjson and adding tests against both orjson and the stdlib json to ensure that the writer works no matter which is imported.

There were two main pieces necessary for the migration:

  1. orjson doesn't support the sort_keys kwarg to dumps; instead its dumps takes an option kwarg which expects bitwise flags. This PR makes the necessary changes to self._json_dumps_args in JsonRpcStreamWriter.__init__ depending on whether orjson is available. (Unnecessary but hopefully helpful: it also adds the separators=(',', ':') kwarg to the stdlib json case so that it matches orjson's behavior of omitting unnecessary spaces.)
  2. orjson.dumps returns bytes, not a string. This PR encodes the dumped JSON if necessary (i.e. in the stdlib case) and uses bytestring % formatting to populate response in JsonRpcStreamWriter.write.

It was tricky to make sure that this worked with or without orjson. The existing test only tests the orjson case if orjson is available on the system on which the tests are run, but I wanted to be sure to test JsonRpcStreamWriter against the stdlib json too. As such, this PR adds a test_writer_stdlib_json test that uses some funky patching to ensure that JsonRpcStreamWriter.write behaves correctly when orjson is unavailable. It's a bit hacky but I confirmed that it does what's intended in the debugger and it seemed better than risking a developer breaking the writer for users who can't import orjson. (Unfortunately I don't think there's any good way to test the writer against orjson if it can't be imported in the dev's environment!)

Loading
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants