Implement reading and writing boolean values

[CoolCat467]

I know nothing in preforming server pings uses boolean values, but this would be super handy to have for handling responses with the forgeData section

[ItsDrike]

forgeData isn't a part of minecraft's official protocol, hence mcstatus doesn't support it. However forgeData seems to just be a part of the status response, as just an additional key in the received JSON. This means that it's already being obtained and is accessible, why would having a bool in the reader/writer classes help? Does the received data contain some bytearrays that need to be read with a buffer (or as we call it connection) reader? Or what's the reason we would need this additional bool operations?

Mcstatus probably isn't the place to introduce these kinds of changes, as you're not expected to use these classes in your code. These classes are considered internal, and they are not a part of the public API. This means they're only expected to be used for internal functionalities, and if they're not necessary for those, they're not needed.

If you do need this kind of change, either implement it locally within your project, or consider checking out https://github.com/py-mine/mcproto, which already supports this behavior, and where the reader/writer classes are indeed a part of the public API: see the implementation (enum def, writer and reader).

[PerchunPak]

I'm looking forward to the forgeData PR!

[PerchunPak]

I agree with ItsDrike: before merging this PR, you need to explain why we even need this, as forgeData is just a JSON attribute which we should parse without touching the Connection classes.

From this comment, I can see how you use the Connection classes to handle forgeData (permalink to the code). However, you don't use it to handle forgeData! It's used for the mysterious attribute d in the response body (code)! This is undocumented in the protocol docs and with this name it's hard to find any information about what it is. I also thought that you modify the dict before passing to this function, but no. You pass it right after getting the status object.

We must be confident that we will even use the code in this PR, so you need to explain how you will use it. Currently, could you explain please what is d and what value it can have?

[kevinkjt2000]

Test changes are a requirement for new features. I concur with the other reviews too. We need to see examples of how this internal function would be used and I suggest that tests are a good way of doing that.

[ItsDrike]

I concur with the other reviews too. We need to see examples of how this internal function would be used
@kevinkjt2000

CoolCat467 already sent a link to an example of where this is useful in #556. However our internal API doesn't need it, and we don't expect others to use the Connection class, as it's not a part of the public API. So in my opinion, this only makes sense if we agree to #556, otherwise this PR can be closed.

(Here's the link to that example from the issue: https://github.com/CoolCat467/StatusBot/blob/main/src/StatusBot/decode_mods.py, it seems that forgeData dict holds some data under "d" key, which is a string in JSON, however it essentially just represents binary data, and the buffer/connection reader/writer classes are indeed necessary to read it).

I wouldn't mind adding this, but again, only if we agree to also add a parser for the forgeData, since otherwise this doesn't really serve us any purpose considering it wouldn't be used internally, and we don't expect it to be used externally.

[CoolCat467]

We must be confident that we will even use the code in this PR, so you need to explain how you will use it. Currently, could you explain please what is d and what value it can have?

Basically, the d field in the forgeData part of a ping response from a forge server is a UTF-16 string that represents binary data containing data like the forge mod loader network version, a big list of channels that all the forge mods use, and a list of mods the server has. Apparently forge ran into an issue a while ago with incredibly large modpacks sending a ton of data in those fields, so much that it exceeded the maximum message size the client can handle. To fix this, the d field was added, which basically acts like compressing the data so it doesn't exceed the max message size, and even if it tries to exceed the max message size, the server checks that and truncates the response if it would be too big.

More information is available in the docstrings in this file from forge itself: https://github.com/MinecraftForge/MinecraftForge/blob/42115d37d6a46856e3dc914b54a1ce6d33b9872a/src/main/java/net/minecraftforge/network/ServerStatusPing.java

Basically I need the ability to read boolean values because the compressed response contains boolean values. Then we could make a forgeData parser so to speak, but it wouldn't really be parsing just decoding the compressed response if the server has it. For older versions of forge, it just put all the data in the forgeData field and you didn't have to decompress it, so this wouldn't be needed for older versions.

[PerchunPak]

Can we use squash strategy for this PR? As the commit history is not really clear.

[ItsDrike]

This can be merged, but only assuming we will later actually add support for showing forgeData. Currently, this will just add an unused method into Connection classes, and since they aren't officially a part of the public API, meaning we don't expect people to use these directly, it doesn't make sense to have it here without actually using it internally.

If this will come with another PR, adding public support for reading forgeData, this can be merged,

[CoolCat467]

Yes, I plan to add a forgeData reader, but I would love to hear feedback about where the interface should be.

[PerchunPak]

Yes, I plan to add a forgeData reader, but I would love to hear feedback about where the interface should be.

Inside JavaStatusResponse class, of course.

[PerchunPak]

Or if you are about where to put the parsing logic, I can send you an example of how I imagine this architecture in Discord.