On Sun, 2016-06-19 at 00:32 +0530, Satyam Zode wrote:

> I made the changes as you mentioned, please find an attached patches. 

Personally, I would suggest that the changes you have separated into
separate patches per file are a single logical change and as such
should all be made in the same commit. Some discussion of this "logical
change" concept from the OpenStack community is here:


> I have removed this temporarily! we already had discussion regarding
> this on IRC. Looking forward to your response! 

I would suggest leaving the range completers in, even with the enforced
sorting by bash itself, I think they are useful. For the upper end of
the completion range, I think go with double the current maximum.

Some further comments on the patches below:

> +dh_auto_build:
> +     debian/diffoscope.bash-completion
> +     debian/diffoscope.1

This will cause the build to fail, those three lines need to all be on
one single line with spaces in between. Did gmail wrap it for you?

dh_auto_build: debian/diffoscope.bash-completion debian/diffoscope.1

> +++ b/debian/clean
> @@ -0,0 +1,3 @@
> +rm -f debian/diffoscope.1
> +dh_clean -O--buildsystem=pybuild

The first two lines of the debian/clean file need fixes. See the manual
page for dh_clean, but the debian/clean file is simply a list of files
(one per line) to be removed by dh_clean from `debian/rules clean`.
The full debian/clean file for diffoscope should look like this:


> +    elif '_ARGCOMPLETE' not in os.environ:

This will trigger the error in the circumstance where you don't have
argcomplete installed and you aren't asking for completion. This means
it will give an error when running it from the command-line if
argcomplete isn't installed. I think we want an error when you don't
have argcomplete installed but you are asking for completion.
To fix this, remove the "not" from this line.

> +        print('ERROR: Argument completion requested but Python argcomplete 
> module not installed', file=sys.stderr)

In another place in the diffoscope codebase, a critical error is
reported using logger.critical before exiting with an error.
I'm not sure if print, logger.critical or logger.error should be used
for this. I guess Lunar or other folks can advise you about this.

logger.critical('Console is unable to print Unicode characters. Set e.g. 




Attachment: signature.asc
Description: This is a digitally signed message part

Reproducible-builds mailing list

Reply via email to