PR#2071: Better help for build/latest-build - koji

koji

#2071 Better help for build/latest-build

Merged 4 years ago by tkopecek. Opened 4 years ago by tkopecek.

tkopecek/koji issue1808 into master

more grammar changes

Mike McLean • 4 years ago

7f27384

fix test

Tomas Kopecek • 4 years ago

ca6ed12

fix grammar

Tomas Kopecek • 4 years ago

b9c3b46

Better help for build/latest-build

Tomas Kopecek • 4 years ago

bc887f2

cli/koji_cli/commands.py

file modified

+27 -3

		`@@ -11,6 +11,7 @@`
		`import re`
		`import stat`
		`import sys`
		`+ import textwrap`
		`import time`
		`import traceback`
		`from collections import OrderedDict, defaultdict`
		`@@ -452,7 +453,19 @@`

		`def handle_build(options, session, args):`
		`"[build] Build a package from source"`
		`- usage = _("usage: %prog build [options] <target> <srpm path or scm url>")`
		`+`
		`+ usage = _("""\`
		`+ usage: %prog build [options] <target> <srpm path or scm url>`
		`+`
		`+ The first option is the build target, not to be confused with the destination`
		`+ tag (where the build eventually lands) or build tag (where the buildroot`
		`+ contents are pulled from).`
		`+`
		`+ You can list all available build targets using the '%prog list-targets' command.`
		`+ More detail can be found in the documentation.`
		`+ https://docs.pagure.org/koji/HOWTO/#package-organization""")`
		`+`
		`+ usage = textwrap.dedent(usage)`
		`parser = OptionParser(usage=get_usage_str(usage))`
		`parser.add_option("--skip-tag", action="store_true",`
		`help=_("Do not attempt to tag package"))`
		`@@ -2325,8 +2338,19 @@`


		`def anon_handle_latest_build(goptions, session, args):`
		`- "[info] Print the latest builds for a tag"`
		`- usage = _("usage: %prog latest-build [options] <tag> <package> [<package> ...]")`
		`+ """[info] Print the latest builds for a tag"""`
		`+ usage = _("""\`
		`+ usage: %prog latest-build [options] <tag> <package> [<package> ...]`
		`+`
		`+ The first option should be the name of a tag, not the name of a build target.`
		`+ If you want to know the latest build in buildroots for a given build target,`
		`+ then you should use the name of the build tag for that target. You can find`
		`+ this value by running '%prog list-targets --name=<target>'`
		`+`
		`+ More information on tags and build targets can be found in the documentation.`
		`+ https://docs.pagure.org/koji/HOWTO/#package-organization""")`
		`+`
		`+ usage = textwrap.dedent(usage)`
		`parser = OptionParser(usage=get_usage_str(usage))`
		`parser.add_option("--arch", help=_("List all of the latest packages for this arch"))`
		`parser.add_option("--all", action="store_true",`

tests/test_cli/test_build.py

file modified

+29 -31

		`@@ -3,14 +3,11 @@`
		`import os`
		`import six`
		`import sys`
		`- try:`
		`- import unittest2 as unittest`
		`- except ImportError:`
		`- import unittest`

		`from koji_cli.commands import handle_build, _progress_callback`
		`+ from . import utils`

		`- class TestBuild(unittest.TestCase):`
		`+ class TestBuild(utils.CliTestCase):`
		`# Show long diffs in error output...`
		`maxDiff = None`

		`@@ -22,6 +19,19 @@`
		`self.options.poll_interval = 0`
		`# Mock out the xmlrpc server`
		`self.session = mock.MagicMock()`
		`+ self.error_format = """Usage: %s build [options] <target> <srpm path or scm url>`
		`+`
		`+ The first option is the build target, not to be confused with the destination`
		`+ tag (where the build eventually lands) or build tag (where the buildroot`
		`+ contents are pulled from).`
		`+`
		`+ You can list all available build targets using the '%s list-targets' command.`
		`+ More detail can be found in the documentation.`
		`+ https://docs.pagure.org/koji/HOWTO/#package-organization`
		`+ (Specify the --help global option for a list of other help options)`
		`+`
		`+ %s: error: {message}`
		`+ """ % (self.progname, self.progname, self.progname)`

		`@mock.patch('sys.stdout', new_callable=six.StringIO)`
		`@mock.patch('koji_cli.commands.activate_session')`
		`@@ -147,11 +157,7 @@`
		`actual_stdout = stdout.getvalue()`
		`actual_stderr = stderr.getvalue()`
		`expected_stdout = ''`
		`- expected_stderr = """Usage: %s build [options] <target> <srpm path or scm url>`
		`- (Specify the --help global option for a list of other help options)`
		`-`
		`- %s: error: Exactly two arguments (a build target and a SCM URL or srpm file) are required`
		`- """ % (progname, progname)`
		`+ expected_stderr = self.format_error_message("Exactly two arguments (a build target and a SCM URL or srpm file) are required")`
		`self.assertMultiLineEqual(actual_stdout, expected_stdout)`
		`self.assertMultiLineEqual(actual_stderr, expected_stderr)`

		`@@ -193,6 +199,14 @@`
		`actual_stdout = stdout.getvalue()`
		`actual_stderr = stderr.getvalue()`
		`expected_stdout = """Usage: %s build [options] <target> <srpm path or scm url>`
		`+`
		`+ The first option is the build target, not to be confused with the destination`
		`+ tag (where the build eventually lands) or build tag (where the buildroot`
		`+ contents are pulled from).`
		`+`
		`+ You can list all available build targets using the '%s list-targets' command.`
		`+ More detail can be found in the documentation.`
		`+ https://docs.pagure.org/koji/HOWTO/#package-organization`
		`(Specify the --help global option for a list of other help options)`

		`Options:`
		`@@ -211,7 +225,7 @@`
		`--repo-id=REPO_ID Use a specific repo`
		`--noprogress Do not display progress of the upload`
		`--background Run the build at a lower priority`
		`- """ % progname`
		`+ """ % (progname, progname)`
		`expected_stderr = ''`
		`self.assertMultiLineEqual(actual_stdout, expected_stdout)`
		`self.assertMultiLineEqual(actual_stderr, expected_stderr)`
		`@@ -257,11 +271,7 @@`
		`actual_stdout = stdout.getvalue()`
		`actual_stderr = stderr.getvalue()`
		`expected_stdout = ''`
		`- expected_stderr = """Usage: %s build [options] <target> <srpm path or scm url>`
		`- (Specify the --help global option for a list of other help options)`
		`-`
		`- %s: error: --arch_override is only allowed for --scratch builds`
		`- """ % (progname, progname)`
		`+ expected_stderr = self.format_error_message("--arch_override is only allowed for --scratch builds")`
		`self.assertMultiLineEqual(actual_stdout, expected_stdout)`
		`self.assertMultiLineEqual(actual_stderr, expected_stderr)`

		`@@ -352,11 +362,7 @@`
		`with self.assertRaises(SystemExit) as cm:`
		`handle_build(self.options, self.session, args)`
		`actual = stderr.getvalue()`
		`- expected = """Usage: %s build [options] <target> <srpm path or scm url>`
		`- (Specify the --help global option for a list of other help options)`
		`-`
		`- %s: error: Unknown build target: target`
		`- """ % (progname, progname)`
		`+ expected = self.format_error_message( "Unknown build target: target")`
		`self.assertMultiLineEqual(actual, expected)`
		`# Finally, assert that things were called as we expected.`
		`activate_session_mock.assert_called_once_with(self.session, self.options)`
		`@@ -403,11 +409,7 @@`
		`with self.assertRaises(SystemExit) as cm:`
		`handle_build(self.options, self.session, args)`
		`actual = stderr.getvalue()`
		`- expected = """Usage: %s build [options] <target> <srpm path or scm url>`
		`- (Specify the --help global option for a list of other help options)`
		`-`
		`- %s: error: Unknown destination tag: dest_tag_name`
		`- """ % (progname, progname)`
		`+ expected = self.format_error_message("Unknown destination tag: dest_tag_name")`
		`self.assertMultiLineEqual(actual, expected)`
		`# Finally, assert that things were called as we expected.`
		`activate_session_mock.assert_called_once_with(self.session, self.options)`
		`@@ -454,11 +456,7 @@`
		`with self.assertRaises(SystemExit) as cm:`
		`handle_build(self.options, self.session, args)`
		`actual = stderr.getvalue()`
		`- expected = """Usage: %s build [options] <target> <srpm path or scm url>`
		`- (Specify the --help global option for a list of other help options)`
		`-`
		`- %s: error: Destination tag dest_tag_name is locked`
		`- """ % (progname, progname)`
		`+ expected = self.format_error_message("Destination tag dest_tag_name is locked")`
		`self.assertMultiLineEqual(actual, expected)`
		`# Finally, assert that things were called as we expected.`
		`activate_session_mock.assert_called_once_with(self.session, self.options)`

tkopecek commented 4 years ago

People can be confused by buildtag/desttag/target. Shed some light in
these commands' helps.

Fixes: https://pagure.io/koji/issue/1808

Metadata Update from @tkopecek:
- Pull-request tagged with: testing-ready

4 years ago

Metadata Update from @tkopecek:
- Pull-request untagged with: testing-ready
- Pull-request tagged with: doc, no_qe

4 years ago

mikem commented 4 years ago

There are some stray double quotes left in the string. I'm seeing this:

$ koji build --help
Usage: lkoji build [options] <target> <srpm path or scm url>

First option is build target (don't confuse it with destination
tag (where build ends) or buildroot (from where dependencies
are installed).

List of all available targets can be acquired by "
'koji list-targets'. For further info about how tags, targets "
and buildroot interact, check the "
https://docs.pagure.org/koji/HOWTO/#package-organization
(Specify the --help global option for a list of other help options)

Options:

mikem commented 4 years ago

I'd probably avoid the nested parenthetical expressions and adjust the grammar a little. Maybe...

The first option is the build target, not to be confused with the destination
tag (where the build eventually lands) or build tag (where the buildroot
contents are pulled from).

You can list all available build targets using the 'koji list-targets'
command. More detail can be found in the documentation.
https://docs.pagure.org/koji/HOWTO/#package-organization

mikem commented 4 years ago

It seems a little odd to format this usage string to match the code where it lies, and then transform it at run time. Also, the lines end up looking short because they are wrapped before col 80 in the code and then dedented by 8.

1 new commit added

fix grammar

4 years ago

tkopecek commented 4 years ago

Any other idea, how to make these strings "nicer"? I've added a commit with grammar fixes and formatted string for larger width. Not sure, if using it without dedent is more readable.

1 new commit added