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

Separate DNS logic into their own staticmethods #203

Closed ItsDrike closed 2 years ago

ItsDrike commented 2 years ago

Addresses #200

As I described in https://github.com/Dinnerbone/mcstatus/issues/200#issuecomment-1026776280, splitting out the DNS lookup functionality into it's own standalone staticmethod that can be called independently without creating a full server instance which can be beneficial if someone would just want to utilize the DNS lookup logic without also trigerring the initialization checks for IP validation and needlessly making an entire sever instance when a simple DNS lookup with this, now separated, logic would suffice.

ItsDrike commented 2 years ago

To further address #200, it may help to also separate the lookup logic into it's own staticmethod (such as resolve_ip) and just call that from the actual alternative lookup constructor. I'm proposing this because even with separate DNS fucnctions, the author would still have to mostly rewrite our entire lookup logic to get the valid IP just to avoid making the final instance. That's not great, especially because they would have to constantly monitor our development, in case we made some changes to lookup, so that they could copy it, which is obviously ridiculous. This further separation would allow for something as easy as: lookup_ip = MinecraftServer.resolve_ip(my_ip).

Should I address this here too or make another PR for that later?

NOTE: Even though the issue was closed already with a different fix, that just addressed another part of that issue, the point I also brought up in there about the fact that we shouldn't need to make an instance to perform these static functions still stays and it's still relevant.

kevinkjt2000 commented 2 years ago

I'm assuming this is the relevant comment from #200:

But the only reason I use mcstatus for DNS info

Hmmm, I am going to draw the line on mcstatus diving too far down being a DNS resolver library. Someone should be using a library/tool that is specific to DNS if that is their main mission. To draw an example from something tangentially related, if I wanted to get the status of something such as Discord, I'd use a tool that understood HTTP to download https://discordstatus.com/, and I wouldn't use that same tool to check the DNS records of discordstatus.com. Likewise, for mcstatus, it is meant to check the status of Minecraft servers and not their DNS records. Sure, DNS is necessary for both of these tools, but it is not their goal.

These dns_* functions do clean up the code and make it easier to follow. I'm open to having these as private functions so that our burden to maintain them as part of the public API is not a requirement.

ItsDrike commented 2 years ago

That's a fair point, yeah I suppose it really isn't our task to do something like that,

I'm open to having these as private functions so that our burden to maintain them as part of the public API is not a requirement.

Python doesn't really have "private functions" per se, best we can do is prefix these methods with _ which is a convention for "this is only used internally and we don't expect you to mess with it, use/alter at your own risk". I'll rename these methods accordingly.

PerchunPak commented 2 years ago

You understood me wrong. Full sentence:

But the only reason I use mcstatus for DNS info, is because you already have the code I wanted to write to check SRV and other stuff.

I also use mcstatus to ping and get information from servers. But at some point I needed to give the user a port from the SRV record, so I started to implement a DNS lookup system. After a while, I noticed that mcstatus was already doing all of this, so I just replaced my entire DNS system with mcstatus.

ItsDrike commented 2 years ago

You understood me wrong. Full sentence: ...

@PerchunPak I don't believe the point was necessarily about how you're using mcstatus, it was more about just saying that we shouldn't do things outside of the scope of just being a library to get status of minecraft servers. While this process may include DNS lookups, it doesn't mean we should be adding and exposing functions to make these lower level lookups available and easily usable for the library users, it simply isn't what this library is for and if someone does want to use it like that, that's on them and we're not responsible for maintaining backwards compatibility on these nor for some exceptions you may encounter while using them.

kevinkjt2000 commented 2 years ago

so I just replaced my entire DNS system with mcstatus.

Seems as if understood perfectly.

it simply isn't what this library is for and if someone does want to use it like that, that's on them and we're not responsible for maintaining backwards compatibility on these nor for some exceptions you may encounter while using them.

Well said! Of course, we could visit properly supporting this as a feature. While working on brand new documentation, I'm also brainstorming what mcstatus' purpose is. This should help design the public API. Perhaps SRV lookups are something that's expected as part of the "status" of a Java Minecraft server. Asserting the existence of that record could be viewed as a possible health check. Probably best to discuss this further in the Discord server until something is concrete enough to make a GitHub issue, so this PR can stay focused on the refactoring that it's doing.