Add as_json to responses
#861
Conversation
| def as_json(self) -> dict: | ||
| as_dict = asdict(self) | ||
| as_dict["motd"] = self.motd.to_minecraft() | ||
| return as_dict |
There was a problem hiding this comment.
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.
PerchunPak
force-pushed
the
add-as-json-to-responses
branch
2 times, most recently
from
a463a49
to
d19f586
Compare
| @@ -101,6 +101,11 @@ def build(cls, *args, **kwargs) -> Self: | |||
| """ | |||
| raise NotImplementedError("You can't use abstract methods.") | |||
|
|
|||
| def as_json(self) -> dict: | |||
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Docstring is missing too. I would highlight the difference between this and raw.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
as_serializable_dict
Serializable in what? Maybe as_json_dict?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
JSON is a string format. Please remove JSON from this function's name that neither consumes a JSON string nor produces a JSON string.
PerchunPak
force-pushed
the
add-as-json-to-responses
branch
from
d19f586
to
2b9731b
Compare
PerchunPak
force-pushed
the
add-as-json-to-responses
branch
from
2b9731b
to
95598e0
Compare
No description provided.