Re: Outreachy - Guix Data Service: implementing basic json output for derivation comparison page

[Luciana Lima Brito]

On Fri, 16 Apr 2021 16:47:10 +0100
Christopher Baines <mail@cbaines.net> wrote:

Hi 


> 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

Thanks, I'll look into it.

> >> 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?

The bindings I was talking about are "outputs", "inputs" and "sources",
the only difference between them are the number of elements each one
has, that is why I simply made a match clause for each one. If you think
it is better I can separate them. Also, do you think it would be clearer
if I move each one inside the json case branch?

> >> 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?

I still don't quite get it. The function I had before was like this
(using inputs as example):

(append-map
 (lambda (label items) 
   (map
    (match-lambda
      ((derivation output)
       (...))
      (or items '()))))
 (list "base" "target" "common")
 (list (assq-ref inputs 'base)
       (assq-ref inputs 'target)
       (assq-ref inputs 'common)))

Which indeed I made based on the html code. However this outputs me
something like:

(("base" derivation output)
 ("target" derivation output)
 ("common" derivation output)
 ...)

where "..." are lots of different ("common" derivation output) lists.

The only way I could think of showing this, was transforming it to a
vector, which gives us the indexes from 0, and inside each one we
would have the label showing where it came from. Is that the way you
think it is better? (is this what your proposed function should
accomplish?)

I think the above output produces a bit less clean json,
that is why I changed this function to the last one I sent you, so I
don't need to pass any labels, because if I pass the base, target, and
common lists separately the output is already correct and we don't need
any vectors.

-- 
Best Regards,

Luciana Lima Brito
MSc. in Computer Science