From: Andy Wingo <wingo@igalia.com>
To: 宋文武 <iyzsong@gmail.com>
Cc: guix-devel@gnu.org
Subject: Re: [PATCH 1/3] utils: Add 'edit-expression'.
Date: Wed, 06 Apr 2016 14:50:13 +0200 [thread overview]
Message-ID: <87r3eic35m.fsf@igalia.com> (raw)
In-Reply-To: <1459939046-10573-1-git-send-email-iyzsong@gmail.com> ("宋文武"'s message of "Wed, 6 Apr 2016 18:37:24 +0800")
Looking really good! A couple nits.
On Wed 06 Apr 2016 12:37, 宋文武 <iyzsong@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.
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
next prev parent reply other threads:[~2016-04-06 12:51 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-04-06 10:37 [PATCH 1/3] utils: Add 'edit-expression' 宋文武
2016-04-06 10:37 ` [PATCH 2/3] utils: Add 'location->source-properties' 宋文武
2016-04-06 10:37 ` [PATCH 3/3] gnu-maintenance: update-package-source: Only update the desired package 宋文武
2016-04-06 12:50 ` Andy Wingo [this message]
2016-04-09 6:12 ` [PATCH][UPDATE] utils: Add 'edit-expression' 宋文武
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=87r3eic35m.fsf@igalia.com \
--to=wingo@igalia.com \
--cc=guix-devel@gnu.org \
--cc=iyzsong@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this external index
https://git.savannah.gnu.org/cgit/guix.git
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.