From: Christopher Baines <mail@cbaines.net>
To: Luciana Lima Brito <lubrito@posteo.net>
Cc: guix-devel@gnu.org
Subject: Re: Outreachy - Guix Data Service: implementing basic json output for derivation comparison page
Date: Thu, 15 Apr 2021 09:46:12 +0100 [thread overview]
Message-ID: <87wnt4x7e3.fsf@cbaines.net> (raw)
In-Reply-To: <20210414164859.7acc631f@lubrito>
[-- Attachment #1: Type: text/plain, Size: 8713 bytes --]
Luciana Lima Brito <lubrito@posteo.net> writes:
> I implemented a basic json output for the derivation comparison page,
> for my first contribution as an Outreachy applicant.
>
> The patch for the code I've changed is attached.
> I'm waiting your reviews :)
Hi Luciana,
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.
Looking at the diff though:
> diff --git a/guix-data-service/web/compare/controller.scm b/guix-data-service/web/compare/controller.scm
> index a6aa198..b7788cb 100644
> --- a/guix-data-service/web/compare/controller.scm
> +++ b/guix-data-service/web/compare/controller.scm
> @@ -584,19 +584,115 @@
> (derivation-differences-data conn
> base-derivation
> target-derivation)))))
> - (case (most-appropriate-mime-type
> - '(application/json text/html)
> - mime-types)
> - ((application/json)
> - (render-json
> - '((error . "unimplemented")) ; TODO
> - #:extra-headers http-headers-for-unchanging-content))
> - (else
> - (render-html
> - #:sxml (compare/derivation
> - query-parameters
> - data)
> - #:extra-headers http-headers-for-unchanging-content)))))))
> + (let ((outputs (assq-ref data 'outputs))
> + (inputs (assq-ref data 'inputs))
> + (sources (assq-ref data 'sources))
> + (system (assq-ref data 'system))
> + (builder (assq-ref data 'builder))
> + (args (assq-ref data 'arguments))
> + (environment-variables (assq-ref data 'environment-variables))
> + (get-derivation-data
> + (lambda (items)
> + (map
> + (match-lambda
> + ((name path hash-alg hash recursive)
> + `(,@(if (null? name)
> + '()
> + `((name . ,name)))
> + ,@(if (null? path)
> + '()
> + `((path . ,path))
> + )
> + ,@(if (or (null? hash-alg) (not (string? hash-alg)))
> + '()
> + `((hash-algorithm . ,hash-alg))
> + )
> + ,@(if (or (null? hash) (not (string? hash)))
> + '()
> + `((hash . ,hash))
> + )
> + ,@(if (null? recursive)
> + '()
> + `((recursive . ,(string=? recursive "t"))))))
> + ((derivation output)
> + `(,@(if (null? derivation)
> + '()
> + `((derivation . ,derivation)))
> + ,@(if (null? output)
> + '()
> + `((output . ,output)))))
> + ((derivation)
> + `(,@(if (null? derivation)
> + '()
> + `((derivation . ,derivation))))))
> + (or items '())))))
> +
> + (let ((base-system (assq-ref system 'base))
> + (target-system (assq-ref system 'target))
> + (common-system (assq-ref system 'common))
> +
> + (base-builder (assq-ref builder 'base))
> + (target-builder (assq-ref builder 'target))
> + (common-builder (assq-ref builder 'common))
> +
> + (base-args (assq-ref args 'base))
> + (target-args (assq-ref args 'target))
> + (common-args (assq-ref args 'common)))
> +
> + (let ((matched-outputs (append-map get-derivation-data
> + (list (assq-ref outputs 'base)
> + (assq-ref outputs 'target)
> + (assq-ref outputs 'common))))
> + (matched-inputs (append-map get-derivation-data
> + (list (assq-ref inputs 'base)
> + (assq-ref inputs 'target))))
> + (matched-sources (append-map get-derivation-data
> + (list (assq-ref sources 'base)
> + (assq-ref sources 'target)
> + (assq-ref sources 'common)))))
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.
(let* ((outputs (assq-ref data 'outputs))
(matched-outputs (append-map get-derivation-data
(list (assq-ref outputs 'base)
(assq-ref outputs 'target)
(assq-ref outputs 'common))))
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?
> + (case (most-appropriate-mime-type
> + '(application/json text/html)
> + mime-types)
> + ((application/json)
> + (render-json
> + `((revision
I'm not sure what revision here referrs to.
> + . ((base
> + . ((derivation . ,base-derivation)))
> + (target
> + . ((derivation . ,target-derivation)))))
> + (outputs
> + . ,((lambda (l) (cond
> + ((= (length l) 3) `((base . ,(first l))
> + (target . ,(second l))
> + (common . ,(third l))))
> + ((= (length l) 2) `((base . ,(first l))
> + (target . ,(second l))))
> + (else `((common . ,(first l))))))
> + matched-outputs))
> + (inputs
> + . ((base . ,(first matched-inputs))
> + (target . ,(second matched-inputs))))
> + (source
> + . ((base . ,(first matched-sources))
> + (target . ,(second matched-sources))
> + (common . ,(third matched-sources))))
> + (system
> + . ((common . ,common-system)))
> + (builder-and-arguments
> + . ((builder . ,common-builder)
> + (arguments
> + . ((base . ,(list->vector
> + base-args))
> + (target . ,(list->vector
> + target-args))))))
> + (environment-variables . ,environment-variables))
> + #:extra-headers http-headers-for-unchanging-content))
> + (else
> + (render-html
> + #:sxml (compare/derivation
> + query-parameters
> + data)
> + #:extra-headers http-headers-for-unchanging-content))))))))))
>
> (define (render-compare/package-derivations mime-types
> query-parameters)
I hope that helps, just let me know if you have any questions,
Chris
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 987 bytes --]
next prev parent reply other threads:[~2021-04-15 8:47 UTC|newest]
Thread overview: 22+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-04-14 19:48 Outreachy - Guix Data Service: implementing basic json output for derivation comparison page Luciana Lima Brito
2021-04-15 8:46 ` Christopher Baines [this message]
2021-04-15 16:09 ` Luciana Lima Brito
2021-04-15 23:19 ` Christopher Baines
2021-04-16 15:07 ` Luciana Lima Brito
2021-04-16 15:47 ` Christopher Baines
2021-04-16 18:46 ` Luciana Lima Brito
2021-04-16 19:17 ` Christopher Baines
2021-04-16 22:47 ` Luciana Lima Brito
2021-04-17 8:40 ` Christopher Baines
2021-04-17 12:48 ` Luciana Lima Brito
2021-04-17 13:11 ` Christopher Baines
2021-04-17 14:08 ` Luciana Lima Brito
2021-04-17 17:45 ` Christopher Baines
2021-04-18 13:12 ` Luciana Lima Brito
2021-04-18 13:19 ` Luciana Lima Brito
2021-04-18 16:34 ` Christopher Baines
2021-04-18 19:12 ` Luciana Lima Brito
2021-04-19 8:26 ` Christopher Baines
2021-04-19 14:04 ` Luciana Lima Brito
2021-04-19 20:20 ` Christopher Baines
2021-04-19 20:56 ` Luciana Lima Brito
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: https://guix.gnu.org/
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=87wnt4x7e3.fsf@cbaines.net \
--to=mail@cbaines.net \
--cc=guix-devel@gnu.org \
--cc=lubrito@posteo.net \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://git.savannah.gnu.org/cgit/guix.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).