[Reproducible-builds] Bug#806891: [Reproducible-commits] [diffoscope] 01/01: Multi-file HTML output

[Jérémy Bobbio]

Joachim Breitner:
>     Multi-file HTML output

Really great idea. :) Thanks for the initial patch!

> +    parser.add_argument('--jquery', metavar='url', dest='jquery_url',
> +                        help='link to the jquery url, with --html-dir. By default, a symlink to /usr/share/javascript/jquery/jquery.js is created')

To make the Suggests a reality, I think it would be better to only
display “By default, a symlink…” if the file is already present on the
filesystem. Otherwise, if `--html-dir` is specified, the software should
exit with an error, asking users to specify `--jquery`.

Related question: is the browser going to visit the subpage if I have
JavaScript disabled? In that case, one other option is to only add the
JavaScript code when we have access to jQuery. I know some people don't
want JavaScript in their browser, so specifying something like
`--no-jquery` or `--jquery=none` could also be a way to turn it off.

> +        # no output desired? print text
> +        if not parsed_args.text_output and not parsed_args.html_output and not parsed_args.html_output_directory:
> +            parsed_args.text_output = "-"

Maybe it would be nicer to write:

    if not any(parsed_args.text_output, parsed_args.html_output, parsed_args.html_output_directory):

> +def output_unified_diff(print_func, directory, anchor, unified_diff):
> +    if directory and len(unified_diff) > Config.general.separate_file_diff_size:
> +        # open a new file for this table
> +        filename="%s.html" % hashlib.md5(anchor.encode('utf-8')).hexdigest()

I'm not entirely sure the anchor as it's working right now will be
unique… 

> +        logger.debug('separate html output for diff of %s (size %d)', anchor, len(unified_diff))
> +        with file_printer(directory, filename) as new_print_func:
> +            output_unified_diff_table(new_print_func, unified_diff)

So I think it would be great to crash here instead of overwrite if the file
aleardy exists. What do you think?

-- 
Lunar                                .''`. 
lunar at debian.org                    : :Ⓐ  :  # apt-get install anarchism
                                    `. `'` 
                                      `-   
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.alioth.debian.org/pipermail/reproducible-builds/attachments/20151202/bbbf5ca6/attachment.sig>