[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[PATCH][UPDATE] utils: Add 'edit-expression'.
From: |
宋文武 |
Subject: |
[PATCH][UPDATE] utils: Add 'edit-expression'. |
Date: |
Sat, 09 Apr 2016 14:12:40 +0800 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/24.5 (gnu/linux) |
Andy Wingo <address@hidden> writes:
> Looking really good! A couple nits.
>
> On Wed 06 Apr 2016 12:37, 宋文武 <address@hidden> writes:
>
>> diff --git a/guix/utils.scm b/guix/utils.scm
>> index de54179..1318dac 100644
>> --- a/guix/utils.scm
>> +++ b/guix/utils.scm
>> +(define* (edit-expression source-properties proc #:key (encoding "UTF-8"))
>> + "Edit the expression specified by SOURCE-PROPERTIES using PROC, which
>> should
>> +be a procedure that take the original expression in string and returns a new
>> +one. ENCODING will be used to interpret all port I/O, it default to UTF-8."
>> + (with-fluids ((%default-port-encoding encoding))
>> + (let*-values (((file line column)
>> + (values
>> + (assoc-ref source-properties 'filename)
>> + (assoc-ref source-properties 'line)
>> + (assoc-ref source-properties 'column)))
>> + ((start end) ; start and end byte positions of the
>> expression
>> + (call-with-input-file file
>> + (lambda (port)
>> + (values
>> + (begin (while (not (and (= line (port-line port))
>> + (= column (port-column
>> port))))
>> + (when (eof-object? (read-char port))
>> + (error 'end-of-file file)))
>> + (ftell port))
>> + (begin (read port)
>> + (ftell port))))))
>
> I think this would be clearer as let*:
>
> (let* ((file (assoc-ref source-properties 'filename))
> ...
> (port (open-input-file file))
> (start (begin ... (ftell port)))
> ...
>
>> + ((pre-bv expr post-bv)
>> + (call-with-input-file file
>> + (lambda (port)
>> + (values (get-bytevector-n port start)
>> + (get-string-n port (- end start))
>> + (get-bytevector-all port))))))
>
> But especially here: `values' is not begin, and it doesn't have a
> defined order of evaluation.
Oh, thanks!
>
> I suggest instead of opening the file again, just (seek port 0
> SEEK_SET), and there you go.
OK.
>
> Also I suggest calling it "str" or something because it's not the
> expression -- it's the string representation of the expression.
>
>> + (with-atomic-file-output file
>> + (lambda (port)
>> + (put-bytevector port pre-bv)
>> + (display (proc expr) port)
>
> Here you may want to verify that the result of (proc expr) is readable
> as a Scheme expression, to prevent problems down the line. e.g.
>
> (let ((str* (proc str)))
> (call-with-input-string str*
> (lambda (port)
> (let lp ()
> (let ((exp (read port)))
> (unless (eof-object? exp)
> (lp)))))))
>
> Dunno, as you like :)
I think a simple ‘(call-with-input-string str* read)’ is enough, since
the original expression was from a read. Also I think it can be emtpy
string (return ‘#<eof>’) to remove the expression.
>
> Finally it could be that the file has some other encoding because of a
> "coding: foo" directive or something; probably best to explicitly
> (set-port-encoding! output-port (port-encoding input-port)) or
> something before writing to the port.
OK, but it seems safe here, since ‘#:guess-encoding’ is default to ‘#f’,
so the I/O ports will both using ‘%default-port-encoding’.
Updated patch:
0001-utils-Add-edit-expression.patch
Description: Text Data
Thanks for the guide and review!