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

Organize --help text #389

Merged
merged 7 commits into from
Nov 14, 2024
Merged

Conversation

mattias-p
Copy link
Member

@mattias-p mattias-p commented Nov 8, 2024

Purpose

Make the --help text more focused and organized.

Context

Changes

  • Group options for easier navigation.
  • Limit the contents and clutter of the --help output, but make sure to point to the man page.
  • Include list of severity levels in --help output again (after removal in Make usage documentation consistent and also demoose #371).
  • Update default value declarations in --help output.
  • Make default value declarations clearer and add missing ones.

How to test this PR

Run zonemaster-cli --help and man zonemaster-cli and make sure they're visually alright.

@mattias-p mattias-p added the V-Patch Versioning: The change gives an update of patch in version. label Nov 8, 2024
@mattias-p mattias-p added this to the v2024.2 milestone Nov 8, 2024
matsduf
matsduf previously approved these changes Nov 11, 2024
script/zonemaster-cli Outdated Show resolved Hide resolved
@mattias-p
Copy link
Member Author

I've rebased this onto develop. Please re-review.

tgreenx
tgreenx previously approved these changes Nov 12, 2024
Copy link
Contributor

@tgreenx tgreenx left a comment

Choose a reason for hiding this comment

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

LGTM.
Btw, do you know of a way to display the full man page without compiling the program in the first place? I tried with perldoc and pod2man to no avail.

@mattias-p
Copy link
Member Author

Btw, do you know of a way to display the full man page without compiling the program in the first place?

Yeah, you can run: perldoc -oman script/zonemaster-cli

This should probably be documented somewhere, I'm just not sure where. I guess I could add it to the README.md#Documentation, but maybe there are better options?

@matsduf
Copy link
Contributor

matsduf commented Nov 13, 2024

Yeah, you can run: perldoc -oman script/zonemaster-cli

This should probably be documented somewhere, I'm just not sure where. I guess I could add it to the README.md#Documentation, but maybe there are better options?

I think https://github.com/zonemaster/zonemaster/blob/master/docs/public/using/cli.md should be the right place. That file should be updated anyway since it states

To see all available command line options, use the --help command.

which is not true anymore.

@matsduf
Copy link
Contributor

matsduf commented Nov 13, 2024

* Include list of severity levels in --help output again (after removal in [Make usage documentation consistent and also demoose #371](https://github.com/zonemaster/zonemaster-cli/pull/371)).

For zonemaster-cli --help the severity levels is relevant for --level=LEVEL. I think that e.g. the following would be an improvement:

  Testing Options:
(...)
    --level=LEVEL
        Specify the minimum level (see levels below) of a message to be printed. (default:
        NOTICE)
(...)
  Severity levels from highest to lowest:
    CRITICAL, ERROR, WARNING, NOTICE, INFO, DEBUG, DEBUG2, DEBUG3

@matsduf
Copy link
Contributor

matsduf commented Nov 13, 2024

@matsduf
Copy link
Contributor

matsduf commented Nov 13, 2024

Does this PR or #371 change the actual behavior of --stop-level=LEVEL? As it is with this update it will break in the middle of the test case which is very unlucky for the development of test scenarios and test zones.

The behavior on a server without these PRs:

$ zonemaster-cli --level info --test basic02 --show-testcase mail.protection.outlook.com --raw
   0.00 INFO     Unspecified    GLOBAL_VERSION  version=v6.0.0
   4.54 CRITICAL Basic02        B02_NO_WORKING_NS  domain="mail.protection.outlook.com"
   4.54 ERROR    Basic02        B02_UNEXPECTED_RCODE  ns=ns2-proddns.glbdns.protection.outlook.com/104.47.72.81; rcode=FORMERR
   4.54 ERROR    Basic02        B02_UNEXPECTED_RCODE  ns=ns1-proddns.glbdns.protection.outlook.com/104.47.34.17; rcode=FORMERR

The behavior with these PRs:

$ zonemaster-cli --level info --test basic02 --show-testcase mail.protection.outlook.com --raw
   0.00 INFO     Unspecified    GLOBAL_VERSION  version=v6.0.0
   4.71 CRITICAL Basic02        B02_NO_WORKING_NS  domain="mail.protection.outlook.com"
Exited early: Saw message at level CRITICAL

I really dislike the new behavior. It should be possible to turn it off again, e.g. with --no-stop-level.

Copy link
Contributor

@matsduf matsduf left a comment

Choose a reason for hiding this comment

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

See my comment.

@mattias-p
Copy link
Member Author

Yeah, you can run: perldoc -oman script/zonemaster-cli
This should probably be documented somewhere, I'm just not sure where. I guess I could add it to the README.md#Documentation, but maybe there are better options?

I think https://github.com/zonemaster/zonemaster/blob/master/docs/public/using/cli.md should be the right place.

End users should be able to run man zonemaster-cli, right? AFAIK the perldoc -oman command is only needed in development contexts where you haven't installed the man page. We should have somewhere to put developer instructions. I'll make a suggestion an include it in this PR.

That file should be updated anyway since it states

To see all available command line options, use the --help command.

which is not true anymore.

Good spotting! I would include an update in this PR but I guess I'll have make another one for it.

@matsduf
Copy link
Contributor

matsduf commented Nov 13, 2024

This is how a previous version of zonemaster-cli breaks testing, i.e. after the actual test case:

zonemaster-cli --level info --show-testcase mail.protection.outlook.com --raw
   0.00 INFO     Unspecified    GLOBAL_VERSION  version=v6.0.0
  10.17 INFO     Basic01        B01_PARENT_FOUND  domain=protection.outlook.com; ns_list=ns1-35.azure-dns.com/150.171.10.35;ns1-35.azure-dns.com/2603:1061:0:10::23;ns2-35.azure-dns.net/150.171.16.35;ns2-35.azure-dns.net/2620:1ec:8ec:10::23;ns3-35.azure-dns.org/13.107.222.35;ns3-35.azure-dns.org/2a01:111:4000:10::23;ns4-35.azure-dns.info/13.107.206.35;ns4-35.azure-dns.info/2620:1ec:bda:10::23
  10.17 INFO     Basic01        B01_CHILD_FOUND  domain=mail.protection.outlook.com
  12.45 CRITICAL Basic02        B02_NO_WORKING_NS  domain="mail.protection.outlook.com"
  12.45 ERROR    Basic02        B02_UNEXPECTED_RCODE  ns=ns1-proddns.glbdns.protection.outlook.com/104.47.34.49; rcode=FORMERR
  12.45 ERROR    Basic02        B02_UNEXPECTED_RCODE  ns=ns2-proddns.glbdns.protection.outlook.com/104.47.34.17; rcode=FORMERR
  12.51 CRITICAL Unspecified    CANNOT_CONTINUE  domain=mail.protection.outlook.com

@mattias-p
Copy link
Member Author

I added a commit with documentation on how to view a development version of the man page. I put it in a new file called DEVELOPMENT.md. At the moment this is the only instruction included in it, but my idea is that in time we'll come up with more things to add.

I'll have a look at the --stop-level problem.

@matsduf
Copy link
Contributor

matsduf commented Nov 13, 2024

I added a commit with documentation on how to view a development version of the man page. I put it in a new file called DEVELOPMENT.md. At the moment this is the only instruction included in it, but my idea is that in time we'll come up with more things to add.

Should we really add documents in this repository? Shouldn't that be in the documentation tree?

If added here, the MANIFEST file must be updated.

@mattias-p
Copy link
Member Author

@matsduf I've can't reproduce your --stop-level problem. I tried running the following command on both the current develop branch and this feature branch, and I get completely identical results.

perl -Ilib script/zonemaster-cli --level=info --no-time --show-testcase --raw --no-ipv6 mail.protection.outlook.com

@mattias-p
Copy link
Member Author

@matsduf It seems the problem you found was introduced in #371 which is already merged, so the problem is in the develop branch and not in this PR.

@mattias-p
Copy link
Member Author

As we discussed in the work group meeting the development documentation should not be located here but rather in the zonemaster/zonemaster repository. I've updated this PR to reflect this.

matsduf
matsduf previously approved these changes Nov 14, 2024
mattias-p and others added 7 commits November 14, 2024 14:57
* Add formatting
* Make option arguments more consistent
* Group twin options into the same entry
* Demote option aliases to an option group at the end
* Hide advanced options from --help output.
* Remove examples from SYNOPSIS section, placing them in an EXAMPLES
  section instead.
* Add proper entries to the SYNOPSIS section.
 * Add missing declarations
 * Change nomenclature from on/off to enabled/disabled
 * Update formatting
Co-authored-by: tgreenx <96772376+tgreenx@users.noreply.github.com>
@mattias-p
Copy link
Member Author

I rebased onto latest develop.

@mattias-p mattias-p merged commit 1f40cae into zonemaster:develop Nov 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
V-Patch Versioning: The change gives an update of patch in version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants