Re: [PATCH v4 01/16] gnu: Add perl-db_file.

[Ricardo Wurmus]

Hi Danny,

> gnu: Add perl-db_file.
>
> * gnu/packages/databases.scm (perl-db_file): New variable.

Thank you for the patches!

What follows are some stylistic comments.  As you wrote earlier you will
prepare another version of this patch set, so I thought I’d let you know
about these stylistic things.

This may seem like nitpicking, but it makes the code look more similar
to the rest, which makes future maintenance easier.

> +
> +(define-public perl-db-file
> + (package
> +  (name "perl-db-file")
> +  (version "1.838")
> +  (source
> +    (origin
> +      (method url-fetch)
> +      (uri (string-append
> +             "mirror://cpan/authors/id/P/PM/PMQS/DB_File-"
> +             version
> +             ".tar.gz"))

I would move “version” and “".tar.gz"” to the same line.

> +      (sha256
> +        (base32
> +          "0yp5d5zr8dk9g6xdh7ygi5bq63q7nxvhd58dk2i3ki4nb7yv2yh9"))))

The indentation of this expression looks wrong.  If you are using Emacs
you can indent properly with “TAB” and “C-M-q”, which is bound to the
“indent-sexp” command.

> +  (build-system perl-build-system)
> +  (inputs `(("bdb" ,bdb)))
> +  (native-inputs `(("perl-test-pod" ,perl-test-pod)))

I know that this is how the importer writes things, but I think it’s
clearer to break the line after “inputs” and “native-inputs”, so that
the list of inputs follows on the next line.

> +  (arguments
> +     `(#:phases (modify-phases %standard-phases
> +                  (add-before
> +                   'configure 'modify-config.in
> +                   (lambda* (#:key inputs #:allow-other-keys)
> +                     (substitute* "config.in"
> +                       (("/usr/local/BerkeleyDB") (assoc-ref inputs "bdb")))
> +                     #t)))))

The value of the “arguments” field is indented much too far.  “TAB”
should fix the indentation in Emacs.

I’d also break the line after “#:phases”.

I’d also move the phase labels “'configure” and “'modify-config.in” onto
the same line as “add-before”.

> +  (home-page "http://search.cpan.org/dist/DB_File";)
> +  (synopsis
> +    "Perl5 access to Berkeley DB version 1.x")

Please move these onto the same line.

> +  (description
> +    "DB::File provides Perl bindings to Berkeley DB version 1.x.")

Please check the indentation.  Maybe wrap “DB::File” in address@hidden

> +  (license (package-license perl))))

~~ Ricardo