bug#24450: [PATCHv2] Re: pypi importer outputs strange character series in optional dependency case.

[Ricardo Wurmus]

Hi Maxim,

on to patch number 2!

> From 5f79b0502f62bd1dacc8ea143c1dbd9ef7cfc29d Mon Sep 17 00:00:00 2001
> From: Maxim Cournoyer <address@hidden>
> Date: Thu, 28 Mar 2019 00:26:00 -0400
> Subject: [PATCH 2/9] import: pypi: Do not parse optional requirements from
>  source.
>
> * guix/import/pypi.scm: Export PARSE-REQUIRES.TXT.
> (guess-requirements): Move the READ-REQUIREMENTS procedure to the top level,
> and rename it to PARSE-REQUIRES.TXT.  Move the CLEAN-REQUIREMENT and COMMENT?
> functions inside the READ-REQUIREMENTS procedure.
> (parse-requires.txt): Add a SECTION-HEADER? predicate, and use it to prevent
> parsing optional requirements.
>
> * tests/pypi.scm (test-requires-with-sections): New variable.
> ("parse-requires.txt, with sections"): New test.
> ("pypi->guix-package"): Mute tar output to stdout.

The commit log does not match the changes.  CLEAN-REQUIREMENT is now a
top-level procedure, not a local procedure inside of READ-REQUIREMENTS
as reported in the commit message.  Which is correct?

> +  (call-with-input-file requires.txt
> +    (lambda (port)
> +      (let loop ((result '()))
> +        (let ((line (read-line port)))
> +          ;; Stop when a section is encountered, as sections contains 
> optional

Should be “contain”.

> +          ;; (extra) requirements.  Non-optional requirements must appear
> +          ;; before any section is defined.
> +          (if (or (eof-object? line) (section-header? line))
> +              (reverse result)
> +              (cond
> +               ((or (string-null? line) (comment? line))
> +                (loop result))
> +               (else
> +                (loop (cons (clean-requirement line)
> +                            result))))))))))
> +

I think it would be better to use “match” here instead of nested “let”,
“if” and “cond”.  At least you can drop the “if” and just use cond.

The loop let and the inner let can be merged.


> +(define (parse-requires.txt requires.txt)
> +  "Given REQUIRES.TXT, a Setuptools requires.txt file, return a list of
> +requirement names."
> +  ;; This is a very incomplete parser, which job is to select the 
> non-optional

“which” –> “whose”

> +  ;; dependencies and strip them out of any version information.
> +  ;; Alternatively, we could implement a PEG parser with the (ice-9 peg)
> +  ;; library and the requirements grammar defined by PEP-0508
> +  ;; (https://www.python.org/dev/peps/pep-0508/).

Let’s remove the sentence starting with “Alternatively…”.  We could do
that but we didn’t :)

> +  (define (section-header? line)
> +    ;; Return #t if the given LINE is a section header, #f otherwise.
> +    (let ((trimmed-line (string-trim line)))
> +      (and (not (string-null? trimmed-line))
> +           (eq? (string-ref trimmed-line 0) #\[))))
> +

How about using string-prefix? instead?  This looks more complicated
than it deserves.  You can get rid of string-null? and eq? and string-ref
and all that.

Same here:

> +  (define (comment? line)
> +    ;; Return #t if the given LINE is a comment, #f otherwise.
> +    (eq? (string-ref (string-trim line) 0) #\#))

I’d just use string-prefix? here.

> +(define (clean-requirement s)
> +  ;; Given a requirement LINE, as can be found in a setuptools requires.txt
> +  ;; file, remove everything other than the actual name of the required
> +  ;; package, and return it.
> +  (string-take s (or (string-index s (lambda (chr)
> +                                       (member chr '(#\space #\> #\= #\<))))
> +                     (string-length s))))

“string-take” with “string-length” is not very elegant.  The char
predicate in string-index could better be a char set:

--8<---------------cut here---------------start------------->8---
(define (clean-requirement s)
 (cond
  ((string-index s (char-set #\space #\> #\= #\<)) => (cut string-take s <>))
  (else s)))
--8<---------------cut here---------------end--------------->8---


> ("pypi->guix-package"): Mute tar output to stdout.

Finally, I think it would be better to keep this separate because it’s
really orthogonal to the other changes in this patch.

What do you think?

-- 
Ricardo