Type annotations for canonicaljson #49
Conversation
Tox wasn't running black locally grr
canonicaljson.py
Outdated
| @@ -95,7 +115,7 @@ def iterencode_canonical_json(json_object): | |||
| yield chunk.encode("utf-8") | |||
|
|
|||
|
|
|||
| def encode_pretty_printed_json(json_object): | |||
| def encode_pretty_printed_json(json_object: object) -> bytes: | |||
There was a problem hiding this comment.
Should we update the docstrings to remove the types?
There was a problem hiding this comment.
Yes. I was being lazy.
The docstrings all suggest that the encoding functions are designed to take dictionaries, but the tests have examples of encoding other things e.g. one character strings:
python-canonicaljson/test_canonicaljson.py
Lines 87 to 88 in 16a9335
There was a problem hiding this comment.
Done in ba77219.
pyproject.toml
Outdated
| exclude = "setup.py" | ||
| #mypy_path = "stubs" | ||
|
|
||
| #[[tool.mypy.overrides]] |
There was a problem hiding this comment.
Yes, thanks. (This was copied from Sydent and I didn't go back and tidy this up!)
There was a problem hiding this comment.
And done in 2411f2f.
| # Declare these in the module scope, but they get configured in | ||
| # set_json_library. | ||
| _canonical_encoder = None | ||
| _pretty_encoder = None | ||
| _canonical_encoder: Encoder = None # type: ignore[assignment] |
There was a problem hiding this comment.
Could you do something silly like import json as _json and then set these types to _json.JSONEncoder?
There was a problem hiding this comment.
Maybe---but that might cause further pain having to wrangle with simplejson.
(I was trying to avoid touching any of the import stuff---feels a bit icky.)
There was a problem hiding this comment.
Being completely honest a lot of this is overkill: the main benefit to Synapse is the existence of -> bytes and -> Generator[bytes, None, None] return types.
| @@ -132,7 +152,7 @@ def iterencode_pretty_printed_json(json_object): | |||
| # | |||
| # Note that it seems performance is on par or better using json from the | |||
| # standard library as of Python 3.7. | |||
| import simplejson as json | |||
| import simplejson as json # type: ignore[no-redef] | |||
There was a problem hiding this comment.
Arg, I wish we could figure out a way to kill this, but I don't think we can due to old Synapse versions which unconditionally install it...
There was a problem hiding this comment.
Ahh, I hadn't realised that Synapse imports and uses set_json_library.
IMO the dependencies that we control (canonicaljson, signedjson, matrix-common?) should be specified in Synapse's metadata with semver bounds to avoid this sort of thing.
There was a problem hiding this comment.
IMO the dependencies that we control (canonicaljson, signedjson, matrix-common?) should be specified in Synapse's metadata with semver bounds to avoid this sort of thing.
Absolutely, but we didn't set an upper bound previously and there isn't really a way to go back and fix that (unless maybe once the Python's from those versions are unsupported?) 🤷
The data to be encoded doesn't have to be a dictionary.
toxpasses locally for me.