Luciana Lima Brito <lubrito@posteo.net> writes:

> On Thu, 15 Apr 2021 09:46:12 +0100
> Christopher Baines <mail@cbaines.net> wrote:
>
> Hi,
>
>> I'm not quite sure how to apply this, I'd suggest using git
>> format-patch to generate the file next time as I think there would
>> normally be some metadata along with the diff.
>
> I tried using git format-patch and I got 7 patches from my 7 commits,
> then I generate a single patch output, which is attached.
> The last commit before my modifications is this:
> 410f58cb43f083623885a430700c6818a187cadc

Ok, I looked at the overall diff, and it looks to me like these changes
should probably be one commit.

>> I would consider whether it's useful to have all these let blocks, and
>> whether here is the right place for them.
>
>> Taking a binding like outputs, it's only used in a later let. You can
>> do something like this (with let*) to remove the need to have
>> multiple let blocks.
>
>> Also, since matched-outputs is only used when rendering the JSON, I'd
>> move all the bindings that are only used for the JSON output within
>> that part of the case statement, so that it's clearer that they only
>> apply to that bit of the code.
>>
>> Does that make sense?
>
> I did it, I used the let* and this helped a lot. I also moved
> everything into the case branch of the json.
>
>> I'm not sure what revision here referrs to.
>
> It was a placeholder, but now I removed it.
>
>> I hope that helps, just let me know if you have any questions,
>
> The function get-derivation-data does not depend on anything, don't you
> think it goes better in another place outside render-compare/derivation?

On the get-derivation-data function, I wouldn't use the same function to
process the inputs, outputs and sources. The data for each is different,
so I would separate the code as well.

To avoid having to call a procedure three times, on the base, target and
common items, I'd consider following the same pattern in the HTML
generating code, map over a list of the lists, so something like:

  (map (lambda (name data)
         (cons name
               (match data
                 ((name path hash-alg hash recursive)
                  ...))))
       '(base target common)
       (list (assq-ref outputs 'base)
             (assq-ref outputs 'target)
             (assq-ref outputs 'common)))

Does that make sense?