Hello again! Ricardo Wurmus <ricardo.wur...@mdc-berlin.de> writes:
> Hi Maxim, > > on to patch number 2! Yay! >> From 5f79b0502f62bd1dacc8ea143c1dbd9ef7cfc29d Mon Sep 17 00:00:00 2001 >> From: Maxim Cournoyer <maxim.courno...@gmail.com> >> 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