From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ricardo Wurmus Subject: Re: [PATCH v4 01/16] gnu: Add perl-db_file. Date: Fri, 29 Jul 2016 07:02:58 +0200 Message-ID: <87k2g5f33x.fsf@elephly.net> References: <20160728131932.0387fb87@scratchpost.org> <20160728132302.3b8efe37@scratchpost.org> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit Return-path: Received: from eggs.gnu.org ([2001:4830:134:3::10]:56469) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bSzwz-0000oK-Qt for guix-devel@gnu.org; Fri, 29 Jul 2016 01:03:10 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bSzwu-0004Ne-Qq for guix-devel@gnu.org; Fri, 29 Jul 2016 01:03:08 -0400 Received: from sender163-mail.zoho.com ([74.201.84.163]:24975) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bSzwu-0004NV-HP for guix-devel@gnu.org; Fri, 29 Jul 2016 01:03:04 -0400 In-reply-to: <20160728132302.3b8efe37@scratchpost.org> List-Id: "Development of GNU Guix and the GNU System distribution." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: guix-devel-bounces+gcggd-guix-devel=m.gmane.org@gnu.org Sender: "Guix-devel" To: Danny Milosavljevic Cc: guix-devel@gnu.org 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 “@code{…}”. > + (license (package-license perl)))) ~~ Ricardo