Dinnerbone / mcstatus

A Python class for checking the status of an enabled Minecraft server
http://dinnerbone.com/minecraft/tools/status/
1.11k stars 146 forks source link

Improve library typehints #207

Closed Fyssion closed 2 years ago

Fyssion commented 2 years ago

As discussed in the Discord server, this PR:

Edit: after re-reading PEP-673, I realized that I was mistaken, and that the previous usage of typing.Self was correct. I have reverted this change.

Fyssion commented 2 years ago

Thanks for the review! I have a few comments.

The simplest reason was just the fact that our dependencies (or their internal deps) already rely on typing-extensions on runtime, so even if we did mark it as a development dependency only, it would still be installed on runtime. But of course, that's not a reason for us to require it specifically on runtime, but it may be a reason not to bother, given the other reasons.

I did not realize this project's dependencies required typing-extensions as a runtime requirement. This does make the removal of typing-extensions as a runtime requirement a bit pointless lol.

Another reason is just the simple fact that it's a lot cleaner to have a simple one-line import, in comparison to having that import be in an if block and all of it's usage be only possible by strings (or another __future__.annotations import like in this case). Then again, this is mostly just a personal opinion since I just prefer my imports somewhat clean, if at all possible, and the cost of one more runtime dependency, (especially a very common one that's already there because of other libs) isn't that much imho. Then again, this is just my personal opinion, again I wouldn't mind it too much if it did get merged.

This makes sense. Hiding the import under an if block doesn't look the best, but I thought it made more sense than including it as a runtime dependency. Now that I know typing-extensions will be installed regardless, I can see why it was included as a runtime dependency in this project as well.

If you'd like, I can revert this change. I don't have a strong opinion either way.

Using __future__.annotations basically causes all type-hints to be treated as strings. This is why we don't need to explicitly wrap these type-hints into quotes and make them strings ourselves with it. However this does pose a downside too, which is actually the reason why this (PEP 56) was not yet made default in python 3.10, which was originally the intention.

The problem with this is simple, if we had a function definition like: def foo(x: MyObj[int]), upon inspecting the type-hints on runtime, we'd see the type for x be "MyObj[int]" (a string), rather than the actual MyObj class reference generic over the actual int type. This makes it really annoying to perform any kind of logic based on these type hints on runtime. With it, the only way to actually resolve this string type hint back into the object it represents would be to run exec on it, with a passed dict representing the globals that are present within the file this type-hint was present on.

Then again, I don't suppose we will need to do any runtime type inspections, so this probably won't be be an issue for us, nevertheless though I wanted to point out this limitation as it is pretty important to know about before this gets merged.

I understand using __future__.annotations treats all annotations as strings (or I believe more specifically forward references), and that these need to be explicitly evaluated (with typing.get_type_hints() or a custom implementation). However, as you stated in the final paragraph, I don't think this library will need to evaluate typehints at runtime, so in theory this shouldn't be an issue. I also feel that type-hinting using string literals looks a bit ugly, but that's just my opinion.

ItsDrike commented 2 years ago

If you'd like, I can revert this change. I don't have a strong opinion either way.

I'd prefer the less cluttered way even at the cost of a runtime dependency, especially if it's going to be there anyway. But I can understand not wanting to do that, as there isn't any good reason for us to also depend on it, regardless of our other dependencies. I don't mind it either way, so let's leave this decision to @kevinkjt2000

However, as you stated in the final paragraph, I don't think this library will need to evaluate typehints at runtime, so in theory this shouldn't be an issue.

Indeed, I just wanted to mention it before this gets merged as it isn't necessarily immediately obvious that something like that will happen and it is a slight disadvantage that this introduces, but yeah, it shouldn't affect this project.

I also feel that type-hinting using string literals looks a bit ugly, but that's just my opinion.

I personally don't mind explicitly annotating with strings, but I don't necessarily prefer it either and I can understand why some people wouldn't like that. It's completely fine by me either way, again, leaving this to @kevinkjt2000

kevinkjt2000 commented 2 years ago

The quotes in the type signature were something I had to learn about when I first saw them, and it baffled me that these annotations were not already a default. It is certainly more intuitive to not have to add quotes around types. Looks like Python 3.11 is going to import the annotations feature by default according to the 3.10 docs, so we may as well hop on the breaking-change hype train in advance. https://docs.python.org/3.10/library/__future__.html image

Typing is something that is ignored at runtime by python interpreters, so it makes sense to look for ways to cut out typing-extensions from runtime dependencies. I added it as a runtime dependency initially only because of the import that was unconditional. Now that this PR conditionally imports based on TYPE_CHECKING, it is safe to move typing-extensions to the development dependcy group.