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

CLI: Bedrock support and misc improvements #849

Merged
merged 36 commits into from
Jul 27, 2024

Conversation

katrinafyi
Copy link
Contributor

@katrinafyi katrinafyi commented Jul 16, 2024

  • Support bedrock servers in CLI, by some simple logic to print additional Java information if available.
  • Add ping() method to BedrockServer (via status().latency)
  • Add kind() abstract method to return a string representation of the server kind.
  • Print "Java"/"Bedrock" in CLI outputs.
  • Fix AttributeError when CLI is called with only one argument #846 by setting default function to status.
  • Print MOTDs using terminal ANSI codes in status and query, assuming this input is meant for humans.
  • Print error messages to stderr.
  • Set program exit codes to indicate success/failure.
  • Formatting tweaks: print bare float in ping and omit trailing space in status if no player sample.
  • Quash top-level exceptions into single line, usually due to socket timeout. Maybe this should be limited to timeout/DNS failures? Or configurable via flag?
  • There is also a workaround for JavaServer ping() always fails? ("did not respond with any information") #850, but that should be addressed directly.

Do you also want tests for the CLI? It would serve as a good end-to-end test case.

Edit: fixes #301.

@katrinafyi
Copy link
Contributor Author

katrinafyi commented Jul 17, 2024

Linting should be fixed. It would be nice to document how to run these checks (and/or the GH action could print the violations). (I did it by uncommitting my changes, then poetry run pre-commit, then re-committing the changes. I'm sure this isn't optimal.)

@PerchunPak
Copy link
Member

Linting should be fixed. It would be nice to document how to run these checks (and/or the GH action could print the violations). (I did it by uncommitting my changes, then poetry run pre-commit, then re-committing the changes. I'm sure this isn't optimal.)

Yeah, https://mcstatus.readthedocs.io/en/stable/pages/contributing/ can be significantly improved

@katrinafyi
Copy link
Contributor Author

katrinafyi commented Jul 17, 2024

Ah, I didn't even find that oops! I thought that would be end-user docs, and I was looking in the readme and CONTRIBUTING.md (which doesn't exist). I now see it is also in-repo at docs/pages/contributing.rst

Copy link
Member

@PerchunPak PerchunPak left a comment

Choose a reason for hiding this comment

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

Nice first PR, this is much better than my first

mcstatus/__main__.py Outdated Show resolved Hide resolved
mcstatus/__main__.py Outdated Show resolved Hide resolved
mcstatus/__main__.py Outdated Show resolved Hide resolved
mcstatus/__main__.py Outdated Show resolved Hide resolved
mcstatus/server.py Outdated Show resolved Hide resolved
mcstatus/__main__.py Outdated Show resolved Hide resolved
@ItsDrike ItsDrike mentioned this pull request Jul 17, 2024
mcstatus/querier.py Outdated Show resolved Hide resolved
mcstatus/__main__.py Outdated Show resolved Hide resolved
@katrinafyi
Copy link
Contributor Author

katrinafyi commented Jul 19, 2024

Is there a good server for testing query? demo.mcstatus.io seems to be failing. Edit: filed #857.

@PerchunPak
Copy link
Member

Is there a good server for testing query? demo.mcstatus.io seems to be failing. Edit: filed #857.

I only use play.lbsg.net for testing bedrock in StatusMC, so you probably should just use docker run -d -it -p 25565:25565 -e EULA=TRUE -e ENABLE_QUERY=true itzg/minecraft-server

ValueError might be thrown by programming errors in
json handling, for example.
since this runs both ping() then status(), it can report
precisely when one fails and the other succeeds.

some kludgy logic to switch bedrock too.
mcstatus/__main__.py Outdated Show resolved Hide resolved
mcstatus/__main__.py Outdated Show resolved Hide resolved
mcstatus/__main__.py Outdated Show resolved Hide resolved
mcstatus/server.py Outdated Show resolved Hide resolved
@PerchunPak PerchunPak requested a review from ItsDrike July 24, 2024 07:22
@DAMcraft
Copy link
Contributor

wow, this is really great, except for one issue
tool.poetry.scripts in the pyproject.toml points to mcstatus.__main__:main, which now requires an argument
this leads to the following:
python3 -m mcstatus works like intended
mcstatus (over the binary) throws an error, because it tries running the main() function without any arguments
grafik

i'd simply add a run() function
so currently it says:

if __name__ == "__main__":
    sys.exit(main(sys.argv[1:]))

which i would change to

def run() -> int:
    return main(sys.argv[1:])


if __name__ == "__main__":
    sys.exit(main(sys.argv[1:]))

then you'd simply need to change it to mcstatus.__main__:run in the pyproject.toml and that's it!

@PerchunPak
Copy link
Member

Or even better I think

def run() -> None:
    sys.exit(main(sys.argv[1:]))

if __name__ == "__main__":
    run()

@DAMcraft
Copy link
Contributor

DAMcraft commented Jul 24, 2024

Or even better I think

def run() -> None:
    sys.exit(main(sys.argv[1:]))

if __name__ == "__main__":
    run()

i don't know if that's the best solution, considering that the tool.poetry.scripts already gets wrapped in a sys.exit

so this would basically lead to the code of

def run() -> None:
    sys.exit(main(sys.argv[1:]))
sys.exit(run())

or simplified:

sys.exit(sys.exit(main(sys.argv[1:])))

which is honestly quite a mess

but honestly i don't know

@PerchunPak
Copy link
Member

Didn't know that, then yes, your solution is better

@DAMcraft
Copy link
Contributor

okay this is the best solution i think, thanks!

mcstatus/__main__.py Outdated Show resolved Hide resolved
Co-authored-by: Kevin Tindall <[email protected]>
mcstatus/__main__.py Outdated Show resolved Hide resolved
Copy link
Contributor

@kevinkjt2000 kevinkjt2000 left a comment

Choose a reason for hiding this comment

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

Thanks for another contribution! I only had a few minor things to point out. Really I wanted to just smack that approve button because this new main script is nice.

@kevinkjt2000
Copy link
Contributor

@ItsDrike The green button is tempting me like the one ring. The precious. I wants it.

mcstatus/__main__.py Outdated Show resolved Hide resolved
mcstatus/__main__.py Show resolved Hide resolved
mcstatus/__main__.py Outdated Show resolved Hide resolved
mcstatus/__main__.py Show resolved Hide resolved
@katrinafyi
Copy link
Contributor Author

thank you for your feedback, I have made changes in response to the addressable points.

@ItsDrike
Copy link
Member

Thanks!

happens during server startup, for example
@PerchunPak PerchunPak merged commit a6c9a61 into py-mine:master Jul 27, 2024
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

AttributeError when CLI is called with only one argument CLI doesn't support Bedrock Servers
5 participants