Andy Wingo <wi...@igalia.com> writes:
> Looking really good! A couple nits.
>
> On Wed 06 Apr 2016 12:37, 宋文武 <iyzs...@gmail.com> 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:
>From 7f48e10a37afc9b45daf76db7632d232bed0940b Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?=E5=AE=8B=E6=96=87=E6=AD=A6?= <iyzs...@gmail.com>
Date: Wed, 6 Apr 2016 17:35:13 +0800
Subject: [PATCH] utils: Add 'edit-expression'.
* guix/utils.scm (edit-expression): New procedure.
* tests/utils.scm (edit-expression): New test.
---
guix/utils.scm | 40 ++++++++++++++++++++++++++++++++++++++++
tests/utils.scm | 13 +++++++++++++
2 files changed, 53 insertions(+)
diff --git a/guix/utils.scm b/guix/utils.scm
index de54179..d82589c 100644
--- a/guix/utils.scm
+++ b/guix/utils.scm
@@ -41,6 +41,7 @@
#:use-module (ice-9 regex)
#:use-module (ice-9 match)
#:use-module (ice-9 format)
+ #:use-module ((ice-9 iconv) #:select (bytevector->string))
#:use-module (system foreign)
#:export (bytevector->base16-string
base16-string->bytevector
@@ -86,6 +87,7 @@
split
cache-directory
readlink*
+ edit-expression
filtered-port
compressed-port
@@ -318,6 +320,44 @@ a list of command-line arguments passed to the compression program."
(unless (every (compose zero? cdr waitpid) pids)
(error "compressed-output-port failure" pids))))))
+(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.
+Return #t on success."
+ (with-fluids ((%default-port-encoding encoding))
+ (let* ((file (assq-ref source-properties 'filename))
+ (line (assq-ref source-properties 'line))
+ (column (assq-ref source-properties 'column))
+ (in (open-input-file file))
+ ;; The start byte position of the expression.
+ (start (begin (while (not (and (= line (port-line in))
+ (= column (port-column in))))
+ (when (eof-object? (read-char in))
+ (error (format #f "~a: end of file~%" in))))
+ (ftell in)))
+ ;; The end byte position of the expression.
+ (end (begin (read in) (ftell in))))
+ (seek in 0 SEEK_SET) ; read from the beginning of the file.
+ (let* ((pre-bv (get-bytevector-n in start))
+ ;; The expression in string form.
+ (str (bytevector->string
+ (get-bytevector-n in (- end start))
+ (port-encoding in)))
+ (post-bv (get-bytevector-all in))
+ (str* (proc str)))
+ ;; Verify the edited expression is still a scheme expression.
+ (call-with-input-string str* read)
+ ;; Replace the file with edited expression.
+ (with-atomic-file-output file
+ (lambda (out)
+ (put-bytevector out pre-bv)
+ (display str* out)
+ ;; post-bv maybe the end-of-file object.
+ (when (not (eof-object? post-bv))
+ (put-bytevector out post-bv))
+ #t))))))
+
;;;
;;; Advisory file locking.
diff --git a/tests/utils.scm b/tests/utils.scm
index 6b77255..d0ee02a 100644
--- a/tests/utils.scm
+++ b/tests/utils.scm
@@ -333,6 +333,19 @@
"This is a journey\r\nInto the sound\r\nA journey ...\n")))
(get-string-all (canonical-newline-port port))))
+
+(test-equal "edit-expression"
+ "(display \"GNU Guix\")\n(newline)\n"
+ (begin
+ (call-with-output-file temp-file
+ (lambda (port)
+ (display "(display \"xiuG UNG\")\n(newline)\n" port)))
+ (edit-expression `((filename . ,temp-file)
+ (line . 0)
+ (column . 9))
+ string-reverse)
+ (call-with-input-file temp-file get-string-all)))
+
(test-end)
(false-if-exception (delete-file temp-file))
--
2.6.3
Thanks for the guide and review!
