search

matrix-org / sygnal

Sygnal: reference Push Gateway for Matrix
Apache License 2.0
160 stars 144 forks source link

Add type stubs for opentracing #282

Closed H-Shay closed 2 years ago

H-Shay commented 2 years ago

When looking at how to get Sygnal to 80% precision, I noticed many of the imprecise lines were either from intentional Anys in our code (i.e. Json dicts with Dict[str, Any] as the type and similar situations) or un-typed third-party code, so I tackled stubbing some of those libraries. More to come.

reivilibre commented 2 years ago

Sorry to be the bearer of bad news (though this might save you the hassle of maintaining these later, so maybe it's good news!)

There appears to be a types-opentracing package (this is a common theme; worth checking for types-xxx and xxx-stubs when wanting stubs for a module xxx).

If you remove the lines from mypy.ini that skip opentracing and do pip install types-opentracing, does it lead to great success?

If so, that may be the way to go — I suppose it would need to be added as a dev dependency.

H-Shay commented 2 years ago

If you remove the lines from mypy.ini that skip opentracing and do pip install types-opentracing, does it lead to great success?

It does indeed lead to great success. I will put up a separate PR with these changes, I suppose the best thing to do with this PR is to close/scrap it?

clokep commented 2 years ago

It could be great to use those on Synapse too! Might be worth filing an issue.

reivilibre commented 2 years ago

It does indeed lead to great success. I will put up a separate PR with these changes

woot!

I suppose the best thing to do with this PR is to close/scrap it?

Yes, probably. Sorry. But thanks for taking the time to do it anyway :)

DMRobertson commented 2 years ago

There appears to be a types-opentracing package (this is a common theme; worth checking for types-xxx and xxx-stubs when wanting stubs for a module xxx).

If it's any consolation, the package is apparently really new: first released last Thursday!

H-Shay commented 2 years ago

It could be great to use those on Synapse too! Might be worth filing an issue.

I can just do this today, not a problem.

clokep commented 2 years ago

To cross reference, this was done in #287.