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

> On Fri, 16 Apr 2021 00:19:46 +0100
> Christopher Baines <mail@cbaines.net> wrote:
>
> Hi,
>
>> Ok, I looked at the overall diff, and it looks to me like these
>> changes should probably be one commit.
>
> I don't actually understand what you mean saying it should be one
> commit. Do I have to make my seven commits become a single one? How do
> I do that?

From looking at the content of your commits, I think they should be
merged together.

There's some information about that here for example:

  https://git-scm.com/book/en/v2/Git-Tools-Rewriting-History#_squashing

>> 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.
>
> I understand that, but the logic to map the values for these three
> bindings is the same, wouldn't it generate redundancies implementing
> the same logic separately?

I'm unsure three bindings are you referring to, can you clairfy?

>> 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?
>
> Actually I did it in a similar way before, but it resulted in a list
> with all the values for base, target and common together, in which
> I had to have another way to separate them and render on json, for
> example, I tried appending "base", "target" or "common" names to each
> list (similar to your function?), but them I had to convert this list to
> a vector.

Getting a list with all of the values in individually was possibly due
to using append-map rather than map.

> Calling the function for each separately gave me a cleaner
> output. Also, I think that sometimes you might have more than one
> output for base, target like it does for common, and I fail to see how
> your example function addresses this. In short, I couldn't see the
> benefit of this over calling the function three times. Is it for
> organizational purpose or am I simply wrong? This time I'm just not
> understanding.

It's an organisational thing, code is generally more readable if the
scope for variables is tight and there's less indirection. Defining a
procedure is one form of indirection. It's often really helpful, but I
think it's unnecessary here.

You're right though about the above example not handling data being a
list, I think that's a fixable problem though, rather than the (match
data ...) bit, I'd suggest using map with match-lambda, probably wrapped
with list->vector if you want a vector which will be outputted as a JSON
array.

Does that make sense?