guix-devel
[Top][All Lists]
Advanced

[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:

Attachment: 0001-utils-Add-edit-expression.patch
Description: Text Data


Thanks for the guide and review!

reply via email to

[Prev in Thread] Current Thread [Next in Thread]