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.

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

mcstatus cli: Motd is not JSON serializable #825

Closed
h7x4 opened this issue Jun 9, 2024 · 3 comments · Fixed by #826
Closed

mcstatus cli: Motd is not JSON serializable #825

h7x4 opened this issue Jun 9, 2024 · 3 comments · Fixed by #826
Labels
area: script Changes to the executable CLI script type: bug Something isn't working

Comments

@h7x4
Copy link

h7x4 commented Jun 9, 2024

Not sure how much more there is to write about this, issue should be clear from the output.

Output:

$ mcstatus mc.hypixel.net json
Traceback (most recent call last):
  File "/usr/lib/python3.12/site-packages/mcstatus/__main__.py", line 100, in main
    args.func(server)
  File "/usr/lib/python3.12/site-packages/mcstatus/__main__.py", line 51, in json
    print(json_dumps(data))
  File "/usr/lib/python3.12/json/__init__.py", line 231, in dumps
    return _default_encoder.encode(obj)
  File "/usr/lib/python3.12/json/encoder.py", line 199, in encode
    chunks = self.iterencode(o, _one_shot=True)
  File "/usr/lib/python3.12/json/encoder.py", line 257, in iterencode
    return _iterencode(o, 0)
  File "/usr/lib/python3.12/json/encoder.py", line 179, in default
    raise TypeError(f'Object of type {o.__class__.__name__} '
TypeError: Object of type Motd is not JSON serializable
@ItsDrike ItsDrike added type: bug Something isn't working area: script Changes to the executable CLI script labels Jun 9, 2024
@ItsDrike
Copy link
Member

ItsDrike commented Jun 9, 2024

Yeah, this is caused by #335 which rewritten how Motd is stored. There are multiple ways we could handle this due to how we represent motd now.

The easiest way to resolve this would be to just use motd.raw which also retains backwards compatibility and is matching with what minecraft actually returned.

The other option would be to customize the serialization to work with how the motd is represented by us in code now. This might be a bit nicer to work with for people, but is a breaking change.

If we would want to go with the representation that follows our custom motd parser, we will also need to decide on how it should look in json. All we'd need is to represent the motd.parsed, some of whcih is just strings, but it can also contain some other things, namely colors and formatting enum types.

I think it's enough to just go with the simple fix of using the raw data.

@ItsDrike
Copy link
Member

ItsDrike commented Jun 9, 2024

CC @PerchunPak

You opinion?

@PerchunPak
Copy link
Member

For now, it is easier to just use Motd.raw, but the entire CLI part is pretty much outdated from how objects are represented internally, and in the ideal world CLI would be need to completely rewritten.

Though I am curious how much CLI is used, and I guess if it was broken for a year, it is not used by almost anyone, so I would like to remove it. It is hard to represent current internal objects with JSON.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: script Changes to the executable CLI script type: bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants