From mboxrd@z Thu Jan  1 00:00:00 1970
From: ludo@gnu.org (Ludovic =?utf-8?Q?Court=C3=A8s?=)
Subject: Re: [PATCH] gnu-maintenance: update-package-source: Only update the
	desired package.
Date: Tue, 05 Apr 2016 11:45:56 +0200
Message-ID: <87y48sv163.fsf@gnu.org>
References: <1459830946-2583-1-git-send-email-iyzsong@gmail.com>
Mime-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable
Return-path: <guix-devel-bounces+gcggd-guix-devel=m.gmane.org@gnu.org>
Received: from eggs.gnu.org ([2001:4830:134:3::10]:53276)
	by lists.gnu.org with esmtp (Exim 4.71)
	(envelope-from <ludo@gnu.org>) id 1anNYg-0002Pd-Iu
	for guix-devel@gnu.org; Tue, 05 Apr 2016 05:46:03 -0400
Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)
	(envelope-from <ludo@gnu.org>) id 1anNYd-00006T-9W
	for guix-devel@gnu.org; Tue, 05 Apr 2016 05:46:02 -0400
In-Reply-To: <1459830946-2583-1-git-send-email-iyzsong@gmail.com>
	(=?utf-8?B?IuWui+aWh+atpiIncw==?=
	message of "Tue, 5 Apr 2016 12:35:46 +0800")
List-Id: "Development of GNU Guix and the GNU System distribution."
	<guix-devel.gnu.org>
List-Unsubscribe: <https://lists.gnu.org/mailman/options/guix-devel>,
	<mailto:guix-devel-request@gnu.org?subject=unsubscribe>
List-Archive: <http://lists.gnu.org/archive/html/guix-devel>
List-Post: <mailto:guix-devel@gnu.org>
List-Help: <mailto:guix-devel-request@gnu.org?subject=help>
List-Subscribe: <https://lists.gnu.org/mailman/listinfo/guix-devel>,
	<mailto:guix-devel-request@gnu.org?subject=subscribe>
Errors-To: guix-devel-bounces+gcggd-guix-devel=m.gmane.org@gnu.org
Sender: guix-devel-bounces+gcggd-guix-devel=m.gmane.org@gnu.org
To: =?utf-8?B?5a6L5paH5q2m?= <iyzsong@gmail.com>
Cc: guix-devel@gnu.org

=E5=AE=8B=E6=96=87=E6=AD=A6 <iyzsong@gmail.com> skribis:

> Fixes <http://bugs.gnu.org/22693>.
> Suggested by Andy Wingo.
>
> * guix/upstream.scm (update-package-source): Use a customized 'substitute'
> to work within lines of the package's source.

[...]

> +  (define (substitute+ file start end pattern+procs)
> +    ;; Same as substitute, but within lines from START to END.

Please make it a top-level procedure, for clarity.

> +  (define (package-location-line-start package)
> +    (location-line (package-location package)))
> +
> +  (define (package-location-line-end package)
> +    (define (goto port line column)
> +      (unless (and (=3D (port-column port)) (- column 1)
> +                   (=3D (port-line port) (- line 1)))
> +        (unless (eof-object? (read-char port))
> +          (goto port line column))))
> +
> +    (match (package-location package)
> +      (($ <location> file line column)
> +       (call-with-input-file (search-path %load-path file)
> +         (lambda (port)
> +           (goto port line column)
> +           (match (read port)
> +             (('package _ ...)
> +              (1+ (port-line port)))))))))

I think you should add a =E2=80=98match=E2=80=99 case here, for when (read =
port) returns
something that is not a =E2=80=98package=E2=80=99 form (that could happen.)

The docstring should specify that this can return #f, and callers should
fall back to a wild guess, like:

  (or (package-location-end-line p)
      (+ (package-location-start-line p) 30))

But overall the approach looks good to me!

Could you:

  1. Call these two procedure =E2=80=98package-location-start-line=E2=80=99=
 and
     =E2=80=98package-location-end-line=E2=80=99 (I think it sounds better =
from a
     grammatical standpoint ;-))?

  2. Move them to (guix packages), next to =E2=80=98package-field-location=
=E2=80=99?

  3. Add tests in tests/packages.scm?

The (guix upstream) changes would come in a separate patch.

WDYT?

Thank you!

Ludo=E2=80=99.