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

[Maxim Cournoyer]

Hello again!

Ricardo Wurmus <address@hidden> writes:

> Hi Maxim,
>
> on to patch number 2!

Yay!

>> 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?

Fixed.

>> +  (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”.

Fixed.

>> +          ;; (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.

I'm not sure I understand; wouldn't merging the named let with the plain
let mean adding an extra LINE argument to my LOOP procedure?  I don't
want that.

Also, how could the above code be expressed using "match"? I'm using
predicates which tests for (special) characters in a string; I don't see
how the more primitive pattern language of "match" will enable me to do
the same.

>> +(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”

Fixed, with due diligence reading on English grammar ;-)

>> +  ;; 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 :)

Alright; done!

>> +  (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.

Neat! Adjusted, for both counts.

>> +(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:
>
> (define (clean-requirement s)
>  (cond
>   ((string-index s (char-set #\space #\> #\= #\<)) => (cut string-take s <>))
>   (else s)))

That's nicer, thanks!

>> ("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.

OK, done!

> What do you think?

All good points; I just don't understand the one about using a match
and/or merging the regular "let" with the named "let".

Thanks,

Maxim