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

[Maxim Cournoyer]

Hello!

Continued feedback about your much appreciated comments! :-)

Ricardo Wurmus <address@hidden> writes:

> Maxim Cournoyer <address@hidden> writes:
>
>>>> +          ;; (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.
>
> Let’s forget about merging the nested “let”, because you would indeed
> need to change a few more things.  It’s fine to keep that as it is.  But
> (if … (cond …)) is not pretty.  At least it could be done in one “cond”:
>
>     (cond
>      ((or (eof-object? line) (section-header? line))
>       (reverse result))
>      ((or (string-null? line) (comment? line))
>       (loop result))
>      (else
>       (loop (cons (clean-requirement line)
>                   result))))

Agreed and fixed, thanks.

>> 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.
>
> “match” has support for predicates, so you could do something like this:
>
>     (match line
>      ((or (eof-object) (? section-header?))
>       (reverse result))
>      ((or '() (? comment?))
>       (loop result))
>      (_ (loop (cons (clean-requirement line) result))))

Oh, that's neat! I had no idea that predicates could be used with
"match".  '() would need to be replaced by "" to match the empty
string.  Another gotcha with "match", is that the "or" seems to evaluate
every component, no matter if a early true condition was found; this
resulted in the following error:

--8<---------------cut here---------------start------------->8---
+ (wrong-type-arg
+   "string-trim"
+   "Wrong type argument in position ~A (expecting ~A): ~S"
+   (1 "string" #<eof>)
+   (#<eof>))
result: FAIL
--8<---------------cut here---------------end--------------->8---

Due to the "(or (eof-object) (? section-header?)" match clause
evaluating the section-header? predicate despite the line being an EOF
character.

> This allows you to match “eof-object” and '() directly.  Whenever I see
> “string-null?” I think it might be better to “match” on the empty list
> directly.

string-null? and an empty list are not the same, unless I'm missing something.

> But really, that’s up to you.  I only feel strongly about avoiding “(if
> … (cond …))”.

Due to the problem mentioned above, I stayed with "cond".

Thanks!

Maxim