Re: [PATCH] guix refresh: Add '--key-download'.

[ludo@gnu.org (Ludovic Courtès)]

Nikita Karetnikov <nikita@karetnikov.org> skribis:

>> First the whole string should be enclosed in (_ ...), otherwise xgettext
>> will just extract "~a~a" for translation.
>
> Should I do the same here?
>
> +                  (match arg
> +                    ((or "interactive" "always" "never")
> +                     (alist-cons 'key-download (string->symbol arg)
> +                                 result))

No, command-line arguments are not i18n’d, so let’s stick to this.

>> Perhaps change it to
>
>>   #:key (key-download 'interactive)
>
> I've tried that, but things like (package-update #:key-download
> key-download) don't look right.  Here is a simplified example:
>
> ;; guix/scripts/refresh.scm
> (define* (update-package #:key (key-download 'interactive))
>   (package-update #:key-download key-download))

That’s fine, IMO.

In general, I would avoid having more than one optional arguments,
unless its meaning and position are obvious.

In this case, it’s not obvious IMO, so the best would probably to make
both ‘archive-type’ and ‘key-download’ keyword arguments.

> scheme@(guile-user)> (update-package)
> interactive
> scheme@(guile-user)> (update-package #:key-download 'never)
> never

It just occurred to me that it might be more intuitive to use one of

  'interactive
  #f   ; never download
  _    ; (any other value) always download

> I'm attaching a patch.  Examples (some commands were omitted):

The examples look great!

> # ./pre-inst-env guix refresh -u
>
> [...]
>
> ftp://ftp.gnu.org/.../guile-2.0.9.tar.gz.sig	100.0% of 0.2 KiB
> gpg: Signature made Wed 10 Apr 2013 06:14:45 AM UTC using DSA key ID EA52ECF4
> gpg: Can't check signature: No public key
> Would you like to download this key and add it to your keyring?
> n
> guix refresh: warning: signature verification failed for `guile-2.0.9.tar.gz'
> guix refresh: warning: (could be because the public key is not in your keyring)
>
> Should I prepend "guix refresh: " to the question?

No.

>  (define* (download-tarball store project directory version
> -                           #:optional (archive-type "gz"))
> +                           #:optional (archive-type "gz")
> +                                      (key-download 'interactive))

Rather make this #:key (key-download 'interactive).

> +(define* (package-update store package #:optional (key-download 'interactive))

Ditto.

> +          (and
> +           missing
> +           (case key-download
> +             ((never) #f)
> +             ((always)
> +              (download-and-try-again))
> +             (else
> +              (and (receive?)
> +                   (download-and-try-again)))))))))

I would write ‘missing’ on the same line as ‘and’.

Modulo these details, it seems ready to get it.

Thanks!

Ludo’.