Re: [PATCH] gnu-maintenance: update-package-source: Only update the desired package.

[ludo@gnu.org (Ludovic Courtès)]

宋文武 <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 (= (port-column port)) (- column 1)
> +                   (= (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 ‘match’ case here, for when (read port) returns
something that is not a ‘package’ 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 ‘package-location-start-line’ and
     ‘package-location-end-line’ (I think it sounds better from a
     grammatical standpoint ;-))?

  2. Move them to (guix packages), next to ‘package-field-location’?

  3. Add tests in tests/packages.scm?

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

WDYT?

Thank you!

Ludo’.