Fix pypi import for flake8-array-spacing

Done

Details

2 participants

Ludovic Courtès
Vivien Kraus

Owner: unassigned

Submitted by: Vivien Kraus

Severity: normal

Vivien Kraus wrote on 20 Jan 2022 20:45

Recipients:(address . guix-patches@gnu.org)

Message-ID:87czkmi434.fsf@planete-kraus.eu

Dear guix,

flake8-array-spacing is a funny package because its download URI uses a

base name of flake8_array_spacing. Apparently some other packages have

similar features, appearing as the downcase version.

What do you think?

Best regards,

Vivien

From d8923c394fbe2e8eedf6fa548455d398f0caa022 Mon Sep 17 00:00:00 2001
From: Vivien Kraus <vivien@planete-kraus.eu>
Date: Thu, 20 Jan 2022 20:11:56 +0100
Subject: [PATCH] pypi importer: Convert - to _ in pypi urls if needed.

* guix/import/pypi.scm (find-project-url): New function.
(make-pypi-sexp): Use find-project-url.
---
 guix/import/pypi.scm | 23 ++++++++++++++---------
 1 file changed, 14 insertions(+), 9 deletions(-)

Toggle diff (43 lines)diff --git a/guix/import/pypi.scm b/guix/import/pypi.scm
index b4284f5c33..fd176e65d5 100644
--- a/guix/import/pypi.scm
+++ b/guix/import/pypi.scm
@@ -418,6 +418,19 @@ (define process-requirements
     (values (map process-requirements dependencies)
             (concatenate dependencies))))
 
+(define (find-project-url name pypi-url)
+  "Try different project name substitution until the result is found in
+pypi-url.  Downcase is required for \"Deprecated\" and \"uWSGI\", and
+underscores are required for flake8-array-spacing."
+  (or (find (cut string-contains pypi-url <>)
+            (list name
+                  (string-downcase name)
+                  (string-replace-substring name "-" "_")))
+      (begin
+        (warning (G_ "The project name `~a' does not appear in the pypi URL; you might need to fix the pypi-url declaration in the generated package.  The URL is: ~a~%")
+                 name pypi-url)
+        name)))
+
 (define (make-pypi-sexp name version source-url wheel-url home-page synopsis
                         description license)
   "Return the `package' s-expression for a python package with the given NAME,
@@ -446,15 +459,7 @@ (define (maybe-upstream-name name)
                     (origin
                       (method url-fetch)
                       (uri (pypi-uri
-                             ;; PyPI URL are case sensitive, but sometimes
-                             ;; a project named using mixed case has a URL
-                             ;; using lower case, so we must work around this
-                             ;; inconsistency.  For actual examples, compare
-                             ;; the URLs of the "Deprecated" and "uWSGI" PyPI
-                             ;; packages.
-                             ,(if (string-contains source-url name)
-                                  name
-                                  (string-downcase name))
+                             ,(find-project-url name source-url)
                              version
                              ;; Some packages have been released as `.zip`
                              ;; instead of the more common `.tar.gz`. For
-- 
2.34.0

Ludovic Courtès wrote on 24 Jan 2022 15:05

Recipients:(name . Vivien Kraus)(address . vivien@planete-kraus.eu)(address . 53395@debbugs.gnu.org)

Message-ID:87ee4xut38.fsf@gnu.org

Hello!

Vivien Kraus <vivien@planete-kraus.eu> skribis:

Toggle quote (8 lines)

> From d8923c394fbe2e8eedf6fa548455d398f0caa022 Mon Sep 17 00:00:00 2001

> From: Vivien Kraus <vivien@planete-kraus.eu>

> Date: Thu, 20 Jan 2022 20:11:56 +0100

> Subject: [PATCH] pypi importer: Convert - to _ in pypi urls if needed.

> * guix/import/pypi.scm (find-project-url): New function.

> (make-pypi-sexp): Use find-project-url.

[...]

Toggle quote (13 lines)> +(define (find-project-url name pypi-url)
> +  "Try different project name substitution until the result is found in
> +pypi-url.  Downcase is required for \"Deprecated\" and \"uWSGI\", and
> +underscores are required for flake8-array-spacing."
> +  (or (find (cut string-contains pypi-url <>)
> +            (list name
> +                  (string-downcase name)
> +                  (string-replace-substring name "-" "_")))
> +      (begin
> +        (warning (G_ "The project name `~a' does not appear in the pypi URL; you might need to fix the pypi-url declaration in the generated package.  The URL is: ~a~%")
> +                 name pypi-url)
> +        name)))

As a rule of thumb, warnings are one-line messages (not sentences)

describing the problem. Here you could emit such a warning, followed by

a call to ‘display-hint’, which accepts Texinfo markup.

Could you adjust accordingly?

Also, what about adding a unit test?

Thanks,

Ludo’.

Vivien Kraus wrote on 24 Jan 2022 23:07

Recipients:(name . Ludovic Courtès)(address . ludo@gnu.org)(address . 53395@debbugs.gnu.org)

Message-ID:87mtjk22rn.fsf@planete-kraus.eu

Hi :)

Ludovic Courtès <ludo@gnu.org> writes:

Toggle quote (3 lines)

> As a rule of thumb, warnings are one-line messages (not sentences)

> describing the problem. Here you could emit such a warning, followed by

> a call to ‘display-hint’, which accepts Texinfo markup.

display-hint seems to always add \n\n at the end of the message, is it

intended that way? Should I do one big display-hint instead of 3?

Toggle quote (1 lines)

> Also, what about adding a unit test?

I added two by copying the "foo" example and pretending it’s named "Foo"
and "goo" respectively (fixing the package name in the json data), while
keeping release URLs as /foo-… so the importer should in the first case
recognize "Foo" in the URL and in the second case it should not
recognize "goo" in the URL so it should emit a warning. I didn’t test
the dash-to-underscore transformation, because it would require test
data with a package named "foo-something" but with release URIs
containing "foo_something". That data point does not exist currently in
the test module.

Vivien

Attachment: 0001-pypi-importer-Convert-to-_-in-pypi-urls-if-needed.patch

Ludovic Courtès wrote on 25 Jan 2022 14:19

Recipients:(name . Vivien Kraus)(address . vivien@planete-kraus.eu)(address . 53395@debbugs.gnu.org)

Message-ID:87y234lzqu.fsf@gnu.org

Hi Vivien,

Vivien Kraus <vivien@planete-kraus.eu> skribis:

Toggle quote (7 lines)

> Ludovic Courtès <ludo@gnu.org> writes:

>> As a rule of thumb, warnings are one-line messages (not sentences)

>> describing the problem. Here you could emit such a warning, followed by

>> a call to ‘display-hint’, which accepts Texinfo markup.

> display-hint seems to always add \n\n at the end of the message, is it

> intended that way? Should I do one big display-hint instead of 3?

Yes, just one ‘display-hint’ call, using complete sentences and

paragraphs. The text should provide an explanation and more importantly

a hint as to what can done, like “The generated package definition might

be wrong; please double-check …”.

You can avoid @strong though, because it doesn’t do

anything useful.

Toggle quote (3 lines)

> + (warning

> + (G_ "The project name does not appear verbatim in the pypi URI~%"))

I’d make it:

"project name ~a does not appear verbatim in the PyPI URI~%"

Toggle quote (2 lines)

>> Also, what about adding a unit test?

[...]

Toggle quote (18 lines)> +(test-equal "If the package goo is released as foo, the importer warns"
> +  "warning: The project name does not appear verbatim in the pypi URI
> +hint: The project name is: *goo*
> +
> +hint: The pypi URI is: *`https://example.com/foo-1.0.0.tar.gz'*
> +
> +hint: The pypi-uri declaration in the generated package might need to be changed.
> +
> +"
> +  (call-with-output-string
> +    (lambda (port)
> +      (parameterize ((guix-warning-port port)
> +                     (current-error-port port))
> +        ;; Replace network resources with sample data.
> +        (mock ((guix import utils) url-fetch
> +               (lambda (url file-name)
> +                 (match url

These two tests are really integration tests: they exercise the whole

shebang. I was thinking about having a unit test of just

‘find-project-url’, which is less work (and maintenance :-)) and may be

good enough.

Perhaps you can also change one of the existing python-foo tests to

exercise this bit, for instance by making it “Foo”.

WDYT?

Thanks,

Ludo’.

Vivien Kraus wrote on 25 Jan 2022 17:39

Recipients:(name . Ludovic Courtès)(address . ludo@gnu.org)(address . 53395@debbugs.gnu.org)

Message-ID:87ee4v21u6.fsf@planete-kraus.eu

Here we go!

Ludovic Courtès <ludo@gnu.org> writes:

Toggle quote (5 lines)

> Yes, just one ‘display-hint’ call, using complete sentences and

> paragraphs. The text should provide an explanation and more importantly

> a hint as to what can done, like “The generated package definition might

> be wrong; please double-check …”.

I made a new version for that.

Toggle quote (4 lines)

> You can avoid @strong though, because it doesn’t do

> anything useful.

Okay.

Toggle quote (8 lines)

>> + (warning

>> + (G_ "The project name does not appear verbatim in the pypi URI~%"))

> I’d make it:

> "project name ~a does not appear verbatim in the PyPI URI~%"

Changed.

Toggle quote (28 lines)>
>>> Also, what about adding a unit test?
>
> [...]
>
>> +(test-equal "If the package goo is released as foo, the importer warns"
>> +  "warning: The project name does not appear verbatim in the pypi URI
>> +hint: The project name is: *goo*
>> +
>> +hint: The pypi URI is: *`https://example.com/foo-1.0.0.tar.gz'*
>> +
>> +hint: The pypi-uri declaration in the generated package might need to be changed.
>> +
>> +"
>> +  (call-with-output-string
>> +    (lambda (port)
>> +      (parameterize ((guix-warning-port port)
>> +                     (current-error-port port))
>> +        ;; Replace network resources with sample data.
>> +        (mock ((guix import utils) url-fetch
>> +               (lambda (url file-name)
>> +                 (match url
>
> These two tests are really integration tests: they exercise the whole
> shebang.  I was thinking about having a unit test of just
> ‘find-project-url’, which is less work (and maintenance :-)) and may be
> good enough.

Let’s do both! If you don’t want the integration tests, just delete them

(now or when maintenance becomes too demanding).

Toggle quote (4 lines)

> Perhaps you can also change one of the existing python-foo tests to

> exercise this bit, for instance by making it “Foo”.

I added a test function that can be used by all integration tests. It’s
not perfect (there’s still a lot of code copied from test to test, and
I’m not innocent there too) but I don’t want to change the other tests
beyond trivial things.

Vivien

Attachment: 0001-pypi-importer-Convert-to-_-in-pypi-urls-if-needed.patch

Ludovic Courtès wrote on 26 Jan 2022 16:20

Recipients:(name . Vivien Kraus)(address . vivien@planete-kraus.eu)(address . 53395-done@debbugs.gnu.org)

Message-ID:87ee4uh6az.fsf@gnu.org

Hi,

Vivien Kraus <vivien@planete-kraus.eu> skribis:

Toggle quote (10 lines)> From 8792367617d288e584a703db38c82c749a067c26 Mon Sep 17 00:00:00 2001
> From: Vivien Kraus <vivien@planete-kraus.eu>
> Date: Thu, 20 Jan 2022 20:11:56 +0100
> Subject: [PATCH] pypi importer: Convert - to _ in pypi urls if needed.
>
> * guix/import/pypi.scm (find-project-url): New function.
> (make-pypi-sexp): Use find-project-url.
> * tests/pypi.scm ("Package Foo has a correct pypi-uri of foo"): New test.
> ("If the package goo is released as foo, the importer warns"): New test.

\o/

I took the liberty to remove the integration tests you had added, I

think it’s preferable (especially since there’s a patch refactoring

these bits that Maxime submitted recently…). I mentioned the other

changes to the commit log and added a copyright line for you.

Let me know if I got anything wrong.

Thank you!

Ludo’.

Closed

Your comment

This issue is archived.

To comment on this conversation send an email to 53395@debbugs.gnu.org

is:open	open issues
is:done	closed issues
submitter:<who>	search issue submitter
author:<who>	search by message author
date:yesterday..now	search by issue date
mdate:3m..2d	search by message date