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.
Add types to synapse.util. #10601
Add types to synapse.util. #10601
Changes from 3 commits
ff8f94d
6deee28
3c2b4dd
1d0b435
22df193
e36db3f
db57064
cd15b4b
348f9ff
d081c83
76c3b6b
30ffee4
10bd84f
0c26b7f
715bfdc
1e4632f
05cc10c
1c6704c
c384373
884a8b6
a22f4c0
029bf34
9444ca1
a0aef0b
289df40
8e719ed
34e327d
cd9a68d
b4cded1
6f7fac0
d4afbca
f5cee54
12cfb9a
e69a3d6
9f301ae
e6618d7
b1b4f1b
ea4f7e0
8871674
20d63a0
19a602e
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.
It's probably better to create a real temporary class here:
I think should work?
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 tried something of that sort, it then said (paraphrased): found
Type[SynapseServerProtocol]
expectedType[ServerProtocol]
.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.
Why do you think it is safe to remove this?
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.
last_tb
andei
are dead.can you see a reason why it's like that? It really needs a comment if there's a reason :/
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.
the original was cargo-culted from https://github.com/python/cpython/blob/main/Lib/code.py#L150, if that helps at all.
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.
Technically not, an exception raised here keeps the local frame around to allow inspection (which is how things like Sentry can report the local variables of each frame in the stack).
I don't know if or why that is an important detail (maybe to prevent recursion if the exception thrown goes back via this function? Or just preventing circular references?), but I'm loathed to remove something like that unless we know why it was put there. (Obligatory reference to Chesterton's fence)
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.
Reminds me of http://www.catb.org/jargon/html/magic-story.html.
I did wonder if there was some subtle reason behind it -- that's why I marked this commit as REVIEW after all.
I guess I will reinstate it, your explanation makes sense enough to hint at a possible context for it being there, pity nobody thought of explaining it though (from the original source) :).
(I wonder why they didn't use
del
if there was a good reason for it being here? Nonetheless, I'm minded to leave it as it was -- in the more magic position -- I was just curious if you knew what it was!)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 don't follow this. This method doesn't raise an exception, so how would
last_db
andei
be retained in a stack frame?I think it's worth digging into the history of the cpython code. It looks to me like those lines were introduced before @reivilibre was born, and at the time there was more code where it might have been useful that the traceback be gc-able.
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 was assuming
self.write
could throw, otherwise there's also a question over why it usestry/finally
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.
Is it? It looks like it should accept a
Sequence[str]
? Which suggests our types our wrongThere 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.
True. I suppose the types are an interface detail, and the actual implementation-accepted types can change. Will address.
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'm don't understand what you mean there, sorry?
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.
Jinja2 currently accepts
Iterable[str]
but says it will acceptSequence[str]
.So morally speaking, they might change the implementation that breaks something so that
Iterable[str]
doesn't work anymore, and it would be silly us for not trusting their type annotations.(Hence I've converted our iterator to a
List
.)