Conversation
ItsDrike
added
t: enhancement
ItsDrike
force-pushed
the
enable-unknown-type-reporting
branch
from
6cbd87e to
e434651
Compare
docs/conf.py
Outdated
|
|
||
| # Default options for all autodoc directives | ||
| autodoc_default_options = { | ||
| autodoc_default_options: dict[str, Any] = { |
There was a problem hiding this comment.
I don't see how these kind of type annotations are better
There was a problem hiding this comment.
Yeah, the thing is, pyright would otherwise mark this as dict[str, Unknown] and with Unknown being banned, if a wildcard type is indeed what we want, it must be declared explicitly like this. That's also one of the reasons why these rules might be a bit too strict.
docs/extensions/attributetable.py
Outdated
| # Given an environment, load up a lookup table of | ||
| # full-class-name: objects | ||
| result = {} | ||
| result: dict[str, list[str]] = {} |
There was a problem hiding this comment.
But for these ones it's interesting to have type checking
There was a problem hiding this comment.
I agree, in some cases, being forced to explicitly declare the annotations can be very helpful to the type checker, it is nice that this would now be a requirement, where types can't be easily inferred.
| def msg(self) -> str: | ||
| """Produce a message for this error.""" | ||
| msg_parts = [] | ||
| msg_parts: list[str] = [] |
There was a problem hiding this comment.
I already have things like that all over the place in more-packets
| buffer = Buffer(data) | ||
|
|
||
| expected_object = { # Name ! Level | ||
| expected_object: FromObjectType = { # Name ! Level |
There was a problem hiding this comment.
This I find useful
|
I'm not against these new rules as I feel they are not the most annoying of the bunch, but I don't really know if the extra effort is worth the while. |
|
The worst / most annoying stuff are recursive types, like those in |
|
Type checking is useful when it uses the computer's power to mitigate our forgetfulness, but when it's going the opposite way I find it annoying 😆 . |
py-mine-ci-bot
bot
added
the
s: stale
ItsDrike
force-pushed
the
enable-unknown-type-reporting
branch
from
7fa69fc to
fd4a475
Compare
py-mine-ci-bot
bot
removed
the
s: stale
Seems that basedpyright now supports recursive types properly, so the latest rebase here removes all of those workaround casts for recursive types, making this a lot more pleasant rule. I'm still slightly worried about enabling it though, as it also means that any untyped libs that we may depend on will cause these warnings every time they're used, and these would be pretty annoying to remove, as simply declaring the type e.g. Instead, the only real way to resolve this would be to use an explicit cast e.g. That said, these rules really are handy to have enabled and do improve the typing experience significantly in some cases. Mcproto also doesn't currently rely heavily on any such untyped libraries, and most of the popular libraries would remain usable without issues, as they are usually typed. Also, mcproto is actually fairly unlikely to pick up a lot of runtime dependencies with what it's purpose is, so it might be worth it. Note that there is a way to compromise, and get the "best of both worlds", where instead of enabling these rules outright, making them required and checked in the validation CI & pre-commit, we could use the That said, we shouldn't just jump to this without thinking, this could still produce a lot of clutter diagnostics when opening up a file with a bunch of such untyped calls and it's also losing on the main benefit of type-checking, being the checking part. If this isn't actually enforced, we can still end up with unknown types in various places, leading to worse type-checking. However, if we do decide against a fully enforced approach, this is definitely something to consider before throwing this PR away entirely. |
py-mine-ci-bot
bot
added
the
s: stale
Since we have the reportUnknownParameterType for basedpyright enabled, we can't leave `*args` or `**kwargs` unannotated anyways, so we might as well enforce it properly.
ItsDrike
force-pushed
the
enable-unknown-type-reporting
branch
from
fd4a475 to
928a4e9
Compare
|
py-mine-ci-bot
bot
removed
the
s: stale
ItsDrike
force-pushed
the
main
branch
2 times, most recently
from
2d364cc to
3f883fb
Compare
py-mine-ci-bot
bot
added
the
s: stale
2 participants
This PR is an experiment, just to see how the code-base would look like if we enabled reporting unknown types for basedpyright.
It's more of a prompt for discussion on whether or not this is worth it, I do not intend to merge this, at least not before this discussion takes place.
The issue with these rules is that they can sometimes be incredibly annoying to satisfy, it often means having to use a bunch of type-casts or just ignoring them. However, in many cases, they can also be incredibly useful, as it's good to know when basedpyright can't recognize some type and needs some help in understanding it. Overall, it would definitely improve the type safety, but the question is whether it's even worth it.