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

Add tests for our CLI #877

Open
wants to merge 12 commits into
base: master
Choose a base branch
from
Open

Add tests for our CLI #877

wants to merge 12 commits into from

Conversation

PerchunPak
Copy link
Member

This is the continuation for #865 (AFAIK I don't have push access to that branch).

katrinafyi and others added 5 commits July 27, 2024 12:14
currently makes use of demo.mcstatus.io as a test server,
but it would be good to run a local instance maybe.

also adds output tests for --help, allowing this to be used
in documentation.

s = s.strip()

# drop lines which end in ":". these argparse section headings vary between python versions.
Copy link
Member

@ItsDrike ItsDrike Aug 17, 2024

Choose a reason for hiding this comment

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

Could you elaborate on this? How do these differ across python versions? Would it be possible to instead add a sys.version_info conditional and only perform 1 replace for the older form to normalize into the newer one?

Copy link
Contributor

Choose a reason for hiding this comment

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

Switching on version is frowned upon and, for such a trivial change that's probably not explicitly documented, would require bisecting the versions.

You could maintain a manual map of replacements, I think these are very few in number. They only concern the "positional arguments", "required arguments", etc section headings.

Copy link
Member

Choose a reason for hiding this comment

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

I see, okay then, doing that does seem fairly annoying, so let's not go there. But it might be worth explaining this a bit more in that comment for people looking at the code.

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 don't really know how to clarify it more, is what is done in c01c7ca enough?

.coveragerc Outdated Show resolved Hide resolved
tests/test_cli.py Outdated Show resolved Hide resolved
tests/test_cli.py Outdated Show resolved Hide resolved
tests/test_cli.py Outdated Show resolved Hide resolved
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.

3 participants