bug#54393: [PATCH 0/2] Add 'guix manifest' to "translate" commands to manifests

[Ludovic Courtès]

Hi!

Maxim Cournoyer <maxim.cournoyer@gmail.com> skribis:


[...]

>> +(define (manifest->code* manifest extra-manifests)
>> +  "Like 'manifest->code', but insert a 'concatenate-manifests' call that
>> +concatenates MANIFESTS, a list of expressions."
>> +  (if (null? (manifest-entries manifest))
>> +      (match extra-manifests
>> +        ((one) one)
>> +        (lst   `(concatenate-manifests ,@extra-manifests)))
>> +      (match (manifest->code manifest
>> +                             #:entry-package-version
>> +                             manifest-entry-version-prefix)
>> +        (('begin exp ... last)
>> +         `(begin
>> +            ,@exp
>> +            ,(match extra-manifests
>> +               (() last)
>> +               (_  `(concatenate-manifests
>> +                     (list ,last ,@extra-manifests)))))))))
>
> Should an "else" clause be added here with a more useful error message
> that the default 'no match for x' or similar?  If that'd be totally
> unexpected and a bug, then it's fine as-is.

Yes, it would be a bug.

>> +  (define (validated-spec spec)
>> +    ;; Return SPEC if it's validate package spec.
>
> As this is an action (proc), perhaps it should be named "validate-spec".
> The comment doc should also be worded as "if SPEC is a valid package
> spec" or similar.

I fixed the comment but kept the name ‘validated-spec’, which is an
attempt at suggesting that it’s returns a meaningful value (whereas
‘validate-spec’ sounds like it’s called for effect and returns
*unspecified*).

> Thank you for addressing the suggestion to reuse an existing sub-command
> to try to keep things neatly organized instead of extending the already
> large set of them :-).

Heheh, thanks!

Pushed as b1e7e64f351fa03a66ce1f9776f9ba84cf2c6294 together with a news
entry.

Ludo’.