py-mine / mcstatus

A Python library for checking the status of Minecraft servers
https://mcstatus.readthedocs.io
Apache License 2.0
468 stars 37 forks source link

CLI: Bedrock support and misc improvements #849

Closed katrinafyi closed 1 month ago

katrinafyi commented 1 month ago

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

Edit: fixes #301.

katrinafyi commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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

katrinafyi commented 1 month ago

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

PerchunPak commented 1 month ago

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

ItsDrike commented 1 month ago

Hey, I'm a bit busy now, so I'll leave the convo resolution up to you guys for now, but once you'll approve it, please request a review from me before merging @PerchunPak

DAMcraft commented 1 month ago

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 commented 1 month ago

Or even better I think

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

if __name__ == "__main__":
    run()
DAMcraft commented 1 month ago

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 commented 1 month ago

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

DAMcraft commented 1 month ago

okay this is the best solution i think, thanks!

kevinkjt2000 commented 1 month ago

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

katrinafyi commented 1 month ago

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

ItsDrike commented 1 month ago

Thanks!