diff mbox series

[bug#67898,v2] guix: import: composer: Handle parsing failures.

Message ID 20231219113857.21287-1-ngraves@ngraves.fr
State New
Headers show
Series [bug#67898,v2] guix: import: composer: Handle parsing failures. | expand

Commit Message

Nicolas Graves Dec. 19, 2023, 11:38 a.m. UTC 
* guix/import/composer (latest-release): Handle parsing
failures. Rename package to composer-package for clarity.

Change-Id: I14936c2c6e6a850a32fe56891766ae92b693a295
---
 guix/import/composer.scm | 20 +++++++++++++-------
 1 file changed, 13 insertions(+), 7 deletions(-)

Comments

Ludovic Courtès April 5, 2024, 8:09 p.m. UTC | #1 
Hi,

Nicolas Graves <ngraves@ngraves.fr> skribis:

> * guix/import/composer (latest-release): Handle parsing
> failures. Rename package to composer-package for clarity.
>
> Change-Id: I14936c2c6e6a850a32fe56891766ae92b693a295

[...]

> +    (match composer-package
> +      (#f
> +       (format (current-error-port)
> +               "warning: failed to parse ~a~%"
> +               php-name)

Please use ‘warning’ from (guix diagnostics) instead.

Does #f necessarily indicate a parse error?  Couldn’t we have more
detailed info about the error?  (Not a blocker but it would be nice.)

> +       #f)
> +      (_
> +       (upstream-source

Nitpick: I’d suggest using a plain ‘if’ rather than ‘match’ in this case
since it’s a binary choice without destructuring.

Could you send an updated patch?

Thanks,
Ludo’.
Nicolas Graves April 6, 2024, 12:55 a.m. UTC | #2 
On 2024-04-05 22:09, Ludovic Courtès wrote:

>> +    (match composer-package
>> +      (#f
>> +       (format (current-error-port)
>> +               "warning: failed to parse ~a~%"
>> +               php-name)
>
> Does #f necessarily indicate a parse error?  Couldn’t we have more
> detailed info about the error?  (Not a blocker but it would be nice.)
>

Well if go through the changes, it could fail on each step of the let*,
so we could indeed be more precise, though it eventually comes down to
a fetch or parsing error. Will fix this in a later patch.

Actually I'm probably going to merge the different patches in a single
patch series, that would make it more convenient to review on your side.


>> +       #f)
>> +      (_
>> +       (upstream-source
>
> Nitpick: I’d suggest using a plain ‘if’ rather than ‘match’ in this case
> since it’s a binary choice without destructuring.
>
> Could you send an updated patch?
>
> Thanks,
> Ludo’.
diff mbox series

Patch

diff --git a/guix/import/composer.scm b/guix/import/composer.scm
index 069a950f90..b23af1b91f 100644
--- a/guix/import/composer.scm
+++ b/guix/import/composer.scm
@@ -246,13 +246,19 @@  (define (php-package? package)
 (define* (latest-release package #:key (version #f))
   "Return an <upstream-source> for the latest release of PACKAGE."
   (let* ((php-name (guix-package->composer-name package))
-         (package (composer-fetch php-name #:version version))
-         (version (composer-package-version package))
-         (url (composer-source-url (composer-package-source package))))
-    (upstream-source
-     (package (composer-package-name package))
-     (version version)
-     (urls (list url)))))
+         (composer-package (composer-fetch php-name #:version version)))
+    (match composer-package
+      (#f
+       (format (current-error-port)
+               "warning: failed to parse ~a~%"
+               php-name)
+       #f)
+      (_
+       (upstream-source
+        (package (composer-package-name composer-package))
+        (version (composer-package-version composer-package))
+        (urls (list (composer-source-url
+                     (composer-package-source composer-package)))))))))
 
 (define %composer-updater
   (upstream-updater