Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add as_json to responses #861

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
+48 −1
Open

Add as_json to responses #861

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 20 additions & 1 deletion mcstatus/responses.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
from __future__ import annotations

from abc import ABC, abstractmethod
from dataclasses import dataclass
from dataclasses import dataclass, asdict
from typing import Any, TYPE_CHECKING

from mcstatus.forge_data import ForgeData, RawForgeData
Expand Down Expand Up @@ -101,6 +101,25 @@ def build(cls, *args, **kwargs) -> Self:
"""
raise NotImplementedError("You can't use abstract methods.")

def as_json(self) -> dict:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer the name of this to be as_dict or to_dict. Methods that dump json should return str just like Python's builtin json.dumps https://docs.python.org/3/library/json.html#json.dumps

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Docstring is missing too. I would highlight the difference between this and raw.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also thought about this name, but the difference between our as_json and dataclasses.asdict is that our method returns JSON-serializable dict. User could also do some more processing on returned object, e.g. use nonstandard JSON library (orjson for example).

Added a docstring in https://github.com/py-mine/mcstatus/compare/d19f58699f37a40cb45dc4c4aa435ce05de8c2a6..2b9731b13aa5b90f99863488603947026f711fb7

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To return anything other than a string would be confusing to all that expect it. Python's json.dumps? String. JavaScript's JSON.parse()? String input. Go's marshal/unmarshal? Byte strings. Rust's popular serde json library? You guessed it, strings. Given the overwhelming amount of these examples that I could keep finding, the name cannot indicate JSON is returned because JSON is a serialized string format.

The user could also serialize to something other than JSON. What about as_serializable_dict?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as_serializable_dict

Serializable in what? Maybe as_json_dict?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Serializable would be an adjective in that name. The function is returning a dictionary that is able to be serialized. What about as_json_dict? No, JSON is a string format as mentioned earlier. Serializable data is not limited to only becoming JSON and could instead be handled with pickle, protobuf, or some other serializer.

Copy link
Member

@ItsDrike ItsDrike Sep 2, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I generally agree with kevin here, it definitely shouldn't be as_json, although I don't mind as_json_dict, since the dict that this produces will be a json serializable dict, which can't be said of every dict (not every python object that can be in a general dict is json serializable).

That said, I also don't mind it just being as_dict, or something like as_user_friendly_dict or some other deviation of that general type of name, the ability to serialize it with JSON doesn't need to be apparent from the function name, it can just be mentioned in the docstring. Also, it is true that by making it as_json_dict, it might imply that it's only meant for json, and no other formats, which isn't necessarily the case, like kevin mentioned.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The function is returning a dictionary that is able to be serialized.

The problem is that it is too loose, you cannot make something that works for every single format on the planet.

What about as_json_dict? No, JSON is a string format as mentioned earlier. Serializable data is not limited to only becoming JSON and could instead be handled with pickle, protobuf, or some other serializer.

I mean, this PR tries to create an easier way to serialize in JSON, it doesn't have a goal to be compatible with anything else. Other formats may have their own quirks, so I don't have a goal to make the underlying object be serializable in any possible format (and function name should reflect that). Yes, JSON is a string format, but user may want to use different library for serializing (e.g. orjson) mainly when computing huge amount of answers in parallel (and we do have such users). In such cases, stdin library is often too slow.

Also, it is true that by making it as_json_dict, it might imply that it's only meant for json, and no other formats, which isn't necessarily the case, like kevin mentioned.

If you can hammer in the nail with scissors, it doesn't mean scissors are intended for this. If this method accidentally works for other formats - good, if not - it is not a supported use case. Again, the goal is to provide an easy way to transform a server's answer to JSON. This is because I needed this method multiple times, and our CLI also has to work around that our response classes are no longer JSON serializable.

Copy link
Contributor

@kevinkjt2000 kevinkjt2000 Sep 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

JSON is a string format. Please remove JSON from this function's name that neither consumes a JSON string nor produces a JSON string.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It'd be a shame to keep blocking this PR just because of the name semantics here. I think we should just go with as_dict, which seems perfectly sufficient; we can always clarify in the docstring that the returned dictionary is suitable for JSON serialization.

Perhaps as_json_dict would be a better name, perhaps it wouldn't, regardless though, I don't see a strong reason to hold up merging over that. as_dict is good enough, let's use that and get this merged.

"""Return the dataclass as JSON-serializable :class:`dict`.

Do note that this method doesn't return :class:`string <str>` but
:class:`dict`, so you can do some processing on returned value.

Difference from
:attr:`~mcstatus.responses.JavaStatusResponse.raw` is in that,
:attr:`~mcstatus.responses.JavaStatusResponse.raw` returns raw response
in the same format as we got it. This method returns the response
in a more user-friendly JSON serializable format (for example,
:attr:`~mcstatus.responses.BaseStatusResponse.motd` is returned as a
:func:`Minecraft string <mcstatus.motd.Motd.to_minecraft>` and not
:class:`dict`).
"""
as_dict = asdict(self)
as_dict["motd"] = self.motd.simplify().to_minecraft()
return as_dict
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Though only response classes (JavaStatusResponse and BedrockStatusResponse) have this method, not JavaStatusPlayer. But for classes without motd inside, it is easier to use dataclasses.asdict to be honest.



@dataclass(frozen=True)
class JavaStatusResponse(BaseStatusResponse):
Expand Down
10 changes: 10 additions & 0 deletions tests/responses/test_bedrock.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,16 @@ def test_optional_parameters_is_none(self, field, pop_index):
build = BedrockStatusResponse.build(parameters, 123.0)
assert getattr(build, field) is None

def test_as_json(self, build: BedrockStatusResponse):
assert build.as_json() == {
"gamemode": "Default",
"latency": 123.0,
"map_name": "map name here",
"motd": "§4G§6a§ey§2B§1o§9w§ds§4e§6r",
"players": {"max": 69, "online": 1},
"version": {"brand": "MCPE", "name": "1.18.100500", "protocol": 422},
}


@BaseResponseTest.construct
class TestBedrockStatusPlayers(BaseResponseTest):
Expand Down
18 changes: 18 additions & 0 deletions tests/responses/test_java.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,24 @@ class TestJavaStatusResponse(BaseResponseTest):
def build(self) -> JavaStatusResponse:
return JavaStatusResponse.build(self.RAW) # type: ignore # dict[str, Unknown] cannot be assigned to TypedDict

def test_as_json(self, build: JavaStatusResponse):
ItsDrike marked this conversation as resolved.
Show resolved Hide resolved
assert build.as_json() == {
"enforces_secure_chat": True,
"forge_data": None,
"icon": "data:image/png;base64,foo",
"latency": 0,
"motd": "A Minecraft Server",
"players": {"max": 20, "online": 0, "sample": None},
"raw": {
"description": "A Minecraft Server",
"enforcesSecureChat": True,
"favicon": "data:image/png;base64,foo",
"players": {"max": 20, "online": 0},
"version": {"name": "1.8-pre1", "protocol": 44},
},
"version": {"name": "1.8-pre1", "protocol": 44},
}


@BaseResponseTest.construct
class TestJavaStatusPlayers(BaseResponseTest):
Expand Down
Loading