Source: diffoscope
Severity: minor
Tags: patch upstream
Dear Maintainer,
I've attached a series of patches to improve documentation, in the output of
`--help` and CONTRIBUTING.
This all started because I noticed `--list-tools` took an optional argument,
and I wasn't sure what a good value for LIST_TOOLS was. I set out to
document that, and that's the first patch.
Then while I was at it I made a series of smaller improvements to the
`--help` output, mostly just making things internally consistent, like the
way metavars are written. That's the second patch.
Since these aren't code bugs, I wasn't sure what the best way to submit them
was, so we had a brief chat about that at one of the reproducible builds
meetings a little while ago. I've updated CONTRIBUTING based on our
conversation, encouraging people to send all patches and improvement
requests to the Debian BTS. That's the third patch.
I took an action item to add something about reporting bugs to `--help`, but
in retrospect I'm not really sure that's right. I think I read CONTRIBUTING
before, but because it suggested so many ways to submit improvements, I felt
unsure which to use. I hope the third patch addresses the issue underlying
the action item, giving future contributors more direction about how to
submit improvements.
I've published my working branch at
<https://github.com/brettcs/diffoscope/tree/bcs-help-output> if that helps
to work with.
Thanks,
--Brett
-- System Information:
Debian Release: 8.6
APT prefers stable-updates
APT policy: (500, 'stable-updates'), (500, 'stable')
Architecture: amd64 (x86_64)
Kernel: Linux 4.9.0 (SMP w/4 CPU cores)
Locale: LANG=C, LC_CTYPE=C (charmap=UTF-8) (ignored: LC_ALL set to en_US.utf8)
Shell: /bin/sh linked to /bin/dash
Init: systemd (via /run/systemd/system)
>From 5523352613c2a83a2b4a7405d039254195ebeec9 Mon Sep 17 00:00:00 2001
From: Brett Smith <brettcsm...@brettcsmith.org>
Date: Fri, 30 Dec 2016 23:18:14 -0500
Subject: [PATCH 1/3] diffoscope: Specify choices for --list-tools switch.
This gives the user more information about what they can specify as an
argument, and generates a nicer error message if they give an unrecognized
argument.
With that done, we can simplify the implementation of ListToolsAction,
because argparse has already validated the argument.
---
diffoscope/main.py | 20 ++++++++------------
diffoscope/tools.py | 13 ++++++++-----
2 files changed, 16 insertions(+), 17 deletions(-)
diff --git a/diffoscope/main.py b/diffoscope/main.py
index c374d8d..64e06fa 100644
--- a/diffoscope/main.py
+++ b/diffoscope/main.py
@@ -62,7 +62,11 @@ def create_parser():
parser.add_argument('--version', action='version',
version='diffoscope %s' % VERSION)
parser.add_argument('--list-tools', nargs='?', type=str, action=ListToolsAction,
- help='Show external tools required and exit')
+ metavar='DISTRO', choices=sorted(OS_NAMES),
+ help='Show external tools required and exit. '
+ 'DISTRO can be one of {%(choices)s}. '
+ 'If specified, the output will list packages in that '
+ 'distribution that satisfy these dependencies.')
parser.add_argument('--debug', dest='debug', action='store_true',
default=False, help='Display debug messages')
parser.add_argument('--debugger', action='store_true',
@@ -184,20 +188,12 @@ class ListToolsAction(argparse.Action):
print("External-Tools-Required: ", end='')
print(', '.join(sorted(tool_required.all)))
if os_override:
- if os_override in OS_NAMES.keys():
- os_list = [os_override]
- else:
- print()
- print("No package mapping found for: {} (possible values: {})".format(os_override, ", ".join(sorted(OS_NAMES.keys()))), file=sys.stderr)
- sys.exit(1)
+ os_list = [os_override]
else:
current_os = get_current_os()
- if current_os in OS_NAMES.keys():
- os_list = [current_os]
- else:
- os_list = OS_NAMES.keys()
+ os_list = [current_os] if (current_os in OS_NAMES) else iter(OS_NAMES)
for os in os_list:
- print("Available-in-{}-packages: ".format(OS_NAMES.get(os, os)), end='')
+ print("Available-in-{}-packages: ".format(OS_NAMES[os]), end='')
print(', '.join(sorted(filter(None, {
EXTERNAL_TOOLS.get(k, {}).get(os, None)
for k in tool_required.all
diff --git a/diffoscope/tools.py b/diffoscope/tools.py
index 8393284..8af5aa3 100644
--- a/diffoscope/tools.py
+++ b/diffoscope/tools.py
@@ -17,6 +17,7 @@
# You should have received a copy of the GNU General Public License
# along with diffoscope. If not, see <https://www.gnu.org/licenses/>.
+import collections
import platform
import functools
@@ -28,11 +29,13 @@ from .profiling import profile
# calls
find_executable = functools.lru_cache()(find_executable)
-OS_NAMES = {
- 'arch': 'Arch Linux',
- 'debian': 'Debian',
- 'FreeBSD': 'FreeBSD',
-}
+# The output of --help and --list-tools will use the order of this dict.
+# Please keep it alphabetized.
+OS_NAMES = collections.OrderedDict([
+ ('arch', 'Arch Linux'),
+ ('debian', 'Debian'),
+ ('FreeBSD', 'FreeBSD'),
+])
def tool_required(command):
--
2.1.4
>From 18515811a551a6173bcd3e2928908f43c12157eb Mon Sep 17 00:00:00 2001
From: Brett Smith <brettcsm...@brettcsmith.org>
Date: Sat, 31 Dec 2016 09:40:51 -0500
Subject: [PATCH 2/3] diffoscope: Copyedit --help output.
* Uppercase all the metavars.
* Group together all the "output and exit" switches.
* Use periods consistently in help strings: yes for all the multi-sentence
help, no otherwise.
* Capitalize all the help strings.
---
diffoscope/main.py | 58 ++++++++++++++++++++++++++++++------------------------
1 file changed, 32 insertions(+), 26 deletions(-)
diff --git a/diffoscope/main.py b/diffoscope/main.py
index 64e06fa..bb97b61 100644
--- a/diffoscope/main.py
+++ b/diffoscope/main.py
@@ -56,27 +56,20 @@ except ImportError:
def create_parser():
parser = argparse.ArgumentParser(
- description='Calculate differences between two files or directories.')
+ description='Calculate differences between two files or directories',
+ add_help=False)
parser.add_argument('path1', help='First file or directory to compare')
parser.add_argument('path2', help='Second file or directory to compare')
- parser.add_argument('--version', action='version',
- version='diffoscope %s' % VERSION)
- parser.add_argument('--list-tools', nargs='?', type=str, action=ListToolsAction,
- metavar='DISTRO', choices=sorted(OS_NAMES),
- help='Show external tools required and exit. '
- 'DISTRO can be one of {%(choices)s}. '
- 'If specified, the output will list packages in that '
- 'distribution that satisfy these dependencies.')
parser.add_argument('--debug', dest='debug', action='store_true',
default=False, help='Display debug messages')
parser.add_argument('--debugger', action='store_true',
- help='Open the python debugger in case of crashes.')
- parser.add_argument('--status-fd', dest='status_fd', metavar='N', type=int,
- help='Send machine-readable status to file descriptor N')
+ help='Open the Python debugger in case of crashes')
+ parser.add_argument('--status-fd', dest='status_fd', metavar='FD', type=int,
+ help='Send machine-readable status to file descriptor FD')
parser.add_argument('--progress', dest='progress', action='store_const',
- const=True, help='Show an (approximate) progress bar')
+ const=True, help='Show an approximate progress bar')
parser.add_argument('--no-progress', dest='progress', action='store_const',
- const=False, help='Do not show an (approximate) progress bar')
+ const=False, help='Do not show any progress bar')
group1 = parser.add_argument_group('output types')
group1.add_argument('--text', metavar='OUTPUT_FILE', dest='text_output',
@@ -87,16 +80,16 @@ def create_parser():
'Default: auto, meaning yes if the output is a terminal, otherwise no.')
group1.add_argument('--output-empty', action='store_true',
help='If there was no difference, then output an empty '
- 'diff for each output type that was specified. (For '
- '--text, an empty file is written.)')
+ 'diff for each output type that was specified. In '
+ '--text output, an empty file is written.')
group1.add_argument('--html', metavar='OUTPUT_FILE', dest='html_output',
help='Write HTML report to given file (use - for stdout)')
group1.add_argument('--html-dir', metavar='OUTPUT_DIR', dest='html_output_directory',
help='Write multi-file HTML report to given directory')
- group1.add_argument('--css', metavar='url', dest='css_url',
+ group1.add_argument('--css', metavar='URL', dest='css_url',
help='Link to an extra CSS for the HTML report')
- group1.add_argument('--jquery', metavar='url', dest='jquery_url',
- help='Link to the jquery url, with --html-dir. Specify '
+ group1.add_argument('--jquery', metavar='URL', dest='jquery_url',
+ help='Link to the jQuery url, with --html-dir. Specify '
'"disable" to disable JavaScript. When omitted '
'diffoscope will try to create a symlink to a system '
'installation. Known locations: %s' % ', '.join(JQUERY_SYSTEM_LOCATIONS))
@@ -117,15 +110,15 @@ def create_parser():
Config().max_report_size, 200000)
group2.add_argument('--max-report-child-size', metavar='BYTES',
dest='max_report_child_size', type=int,
- help='In html-dir output, this is the max bytes of '
- 'each child page. (0 to disable, default: %(default)s, '
+ help='In --html-dir output, this is the max bytes of '
+ 'each child page (0 to disable, default: %(default)s, '
'remaining in effect even with --no-default-limits)',
default=Config().max_report_child_size).completer=RangeCompleter(0,
Config().max_report_child_size, 50000)
group2.add_argument('--max-diff-block-lines', dest='max_diff_block_lines',
metavar='LINES', type=int,
help='Maximum number of lines output per diff block. '
- 'In html-dir output, we use %d * this number instead, '
+ 'In --html-dir output, we use %d times this number instead, '
'taken over all pages. (0 to disable, default: %d)' %
(Config().max_diff_block_lines_html_dir_ratio,
Config().max_diff_block_lines),
@@ -134,8 +127,8 @@ def create_parser():
group2.add_argument('--max-diff-block-lines-parent', dest='max_diff_block_lines_parent',
metavar='LINES', type=int,
help='In --html-dir output, this is maximum number of '
- 'lines output per diff block on the parent page, '
- 'before spilling it into child pages. (0 to disable, '
+ 'lines output per diff block on the parent page '
+ 'before spilling it into child pages (0 to disable, '
'default: %(default)s, remaining in effect even with '
'--no-default-limits)',
default=Config().max_diff_block_lines_parent).completer=RangeCompleter(0,
@@ -145,7 +138,7 @@ def create_parser():
help='Maximum number of lines saved per diff block. '
'Most users should not need this, unless you run out '
'of memory. This truncates diff(1) output before even '
- 'trying to emit it in a report; also affects --text '
+ 'trying to emit it in a report. This also affects --text '
'output. (0 to disable, default: 0)',
default=0).completer=RangeCompleter(0, 0, 200)
@@ -159,12 +152,25 @@ def create_parser():
400, 20)
group3.add_argument('--max-diff-input-lines', dest='max_diff_input_lines',
metavar='LINES', type=int,
- help='Maximum number of lines fed to diff(1). '
+ help='Maximum number of lines fed to diff(1) '
'(0 to disable, default: %d)' %
Config().max_diff_input_lines,
default=None).completer=RangeCompleter(0,
Config().max_diff_input_lines, 5000)
+ group4 = parser.add_argument_group('information commands')
+ group4.add_argument('--help', '-h', action='help',
+ help="Show this help and exit")
+ group4.add_argument('--version', action='version',
+ version='diffoscope %s' % VERSION,
+ help="Show program's version number and exit")
+ group4.add_argument('--list-tools', nargs='?', type=str, action=ListToolsAction,
+ metavar='DISTRO', choices=OS_NAMES,
+ help='Show external tools required and exit. '
+ 'DISTRO can be one of {%(choices)s}. '
+ 'If specified, the output will list packages in that '
+ 'distribution that satisfy these dependencies.')
+
if not tlsh:
parser.epilog = 'File renaming detection based on fuzzy-matching is currently disabled. It can be enabled by installing the "tlsh" module available at https://github.com/trendmicro/tlsh'
if argcomplete:
--
2.1.4
>From 45a5267b16eb178151835cf698e44bf0d2a6f03c Mon Sep 17 00:00:00 2001
From: Brett Smith <brettcsm...@brettcsmith.org>
Date: Fri, 20 Jan 2017 13:12:33 -0500
Subject: [PATCH 3/3] CONTRIBUTING: Refresh instructions for contributing to
diffoscope.
The previous instructions suggested multiple ways to contribute. Personally
I didn't feel sure which one was really right to use. These encourage the
reader to send everything to the BTS, based on the conversation at
<http://meetbot.debian.net/reproducible-builds/2017/reproducible-builds.2017-01-03-18.00.log.html#l-38>.
I also tried to make the instructions a little less Debian-specific by
pointing people to the instructions for sending reports by e-mail, and then
mentioning reportbug as a helper for people who have it.
---
CONTRIBUTING | 36 +++++++++++++++++++-----------------
1 file changed, 19 insertions(+), 17 deletions(-)
diff --git a/CONTRIBUTING b/CONTRIBUTING
index 7dd9936..0efbfbf 100644
--- a/CONTRIBUTING
+++ b/CONTRIBUTING
@@ -1,30 +1,32 @@
-=== Contributing code to this project
-
-It's helpful to track fixes or new features via wishlist bugs against the
-'diffoscope' package, eg with the 'reportbug' tool ('devscripts' package).
-
-The code is available in the link:https://anonscm.debian.org/git/reproducible/diffoscope.git/tree/[diffoscope.git repository].
-
-Patches can be submitted as requests to pull from a publicly-visible
-git repository (this is the prefered way) communicated via IRC or mail,
-or completly via mail (using git format-patch, see below).
-If appropriate, please make a topic branch based on the 'master' branch.
-
-You can send patches or requests to the link:diffosc...@lists.reproducible-builds.org[development list],
-or to the tracking bug: <bugnumber>@bugs.debian.org.
+=== Contributing to this project
+
+The preferred way to report bugs about diffoscope, as well as suggest fixes
+and requests for improvements, is to submit reports to the Debian bug
+tracker for the 'src:diffoscope' package. You can do this over e-mail;
+link:https://www.debian.org/Bugs/Reporting[the bug tracker has instructions
+for sending in your report.] If you're on a Debian-based system, you can
+install and use the 'reportbug' package to help walk you through the
+process.
+
+You can submit patches to the Debian bug tracker too. Start by cloning the
+link:https://anonscm.debian.org/git/reproducible/diffoscope.git/tree/[diffoscope
+Git repository]. Make your changes and commit them as you normally would.
+Then you can use Git's 'format-patch' command to save your changes as a
+series of patches that can be attached to the report you submit.
One possible workflow:
----
git clone git://anonscm.debian.org/reproducible/diffoscope.git
+ cd diffoscope
git checkout origin/master -b <topicname>
# <edits>
git commit -a
git format-patch -M origin/master
-
- reportbug qa.debian.org
- # <describe the issue, attach the patch>
----
+The 'format-patch' command will make a series of '.patch' files in your
+checkout. Attach these files to your submission in your e-mail client or
+reportbug.
=== Uploading the package
--
2.1.4
