Fix pypi import for flake8-array-spacing

Done

Details

2 participants

Ludovic Courtès
Vivien Kraus

Owner: unassigned

Submitted by: Vivien Kraus

Severity: normal

Debbugs page

Vivien Kraus wrote 3 years ago

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 3 years ago

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 3 years ago

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 3 years ago

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 3 years ago

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 3 years ago

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

To respond to this issue using the mumi CLI, first switch to it

mumi current 53395

Then, you may apply the latest patchset in this issue (with sign off)

mumi am -- -s

Or, compose a reply to this issue

mumi compose

Or, send patches to this issue

mumi send-email *.patch

You may also tag this issue. See list of standard tags. For example, to set the confirmed and easy tags

mumi command -t +confirmed -t +easy

Or, remove the moreinfo tag and set the help tag

mumi command -t -moreinfo -t +help

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