This repository has been archived by the owner on Apr 26, 2024. It is now read-only.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Return read-only collections from
@cached
methods #13755Return read-only collections from
@cached
methods #13755Changes from all commits
45821e1
3dad3b0
ed43ae3
1248c1e
ed6f1fe
1fe7d82
7f0f531
9d23871
d7f84d7
e76eef1
08e9135
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This goes straight into the
SyncResult
and is pretty much only going to be JSON-serialized from here on.Having to take a copy to make mypy happy is unfortunate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At least it's a shallow copy 🤷
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd be curious of the error here? Is something in our code unhappy or is it a type hint of like
json.dump
?I think it'd be OK to add a
cast
with a comment explaining it is for performance.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a type hint problem. Only
List
s andTuple
s are JSON-serializable, except they're mutable types. I would have updated the definition ofSyncResult
otherwise.