Re: [PATCH 1/3] utils: Add 'edit-expression'.

[Andy Wingo]

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.

I suggest instead of opening the file again, just (seek port 0
SEEK_SET), and there you go.

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

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.

All that said -- looks good to me, thank you for putting in the effort!

Andy