From mboxrd@z Thu Jan 1 00:00:00 1970 From: ng0 Subject: Re: [PATCH] gnu: Add lmms Date: Thu, 23 Feb 2017 11:30:25 +0000 Message-ID: <20170223113025.hug4ysmujjnipbqr@wasp> References: <412b9673cde38582067020f679ca9e70@lepiller.eu> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Return-path: Received: from eggs.gnu.org ([2001:4830:134:3::10]:54551) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1cgrZR-0003NS-Ae for guix-devel@gnu.org; Thu, 23 Feb 2017 06:28:26 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1cgrZN-0007ZX-Cc for guix-devel@gnu.org; Thu, 23 Feb 2017 06:28:25 -0500 Received: from perdizione.investici.org ([2001:41d0:2:33d0::19]:34415) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1cgrZN-0007ZG-3S for guix-devel@gnu.org; Thu, 23 Feb 2017 06:28:21 -0500 Content-Disposition: inline In-Reply-To: <412b9673cde38582067020f679ca9e70@lepiller.eu> 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: julien lepiller Cc: guix-devel@gnu.org On 17-02-23 10:15:47, julien lepiller wrote: > Le 2017-02-23 00:18, Rodger Fox a =C3=A9crit=C2=A0: > > Here is another package. I think I got the commit message right this > > time. Let me know if I missed anything. >=20 > Hi! >=20 > I wanted to try this software for while now, so you're great :) >=20 > I added some comments on your patch below: >=20 > > From 8e2757bee584f4686e02da512662fb512b05c037 Mon Sep 17 00:00:00 200= 1 > > From: Rodger Fox > > Date: Wed, 22 Feb 2017 15:08:30 -0800 > > Subject: [PATCH] gnu: Add lmms. * gnu/packages/music.scm (lmms): New > > variable. > The message is correct, but it lacks returns between "Add lmms." and "* > gnu/packages". Have a look at git log for some examples. >=20 > > + (sha256 > > + (base32 > > + "1g76z7ha3hd53vbqaq9n1qg6s3lw8zzaw51iny6y2bz0j1xqwcsr")))) > There's a tab in the indentation, please use spaces. >=20 > > + (build-system cmake-build-system) > > + (arguments `(#:tests? #f ; No tests to run. > > + #:validate-runpath? #f)) > There's a tab here too, and it should rather look like this: >=20 > (arguments > `(#:tests? #f > #:validate-runpath? #f)) >=20 > Why do you need to disable runpath validation? >=20 > > + (native-inputs > > + `(("pkg-config" ,pkg-config))) > > + (inputs > Indentation is off by one (inputs should be aligned with native-inputs)= . >=20 > > + (description "LMMS is a digital audio workstation. It includes > > tools for sequencing melodies and beats and for mixing and arranging > > songs. It includes instruments based on audio samples and various sof= t > > sythesizers. It can receive input from a MIDI keyboard.") > This line is way too long, please break it. Also please use two spaces > between sentences. It's a good idea to run "./pre-inst-env guix lint PACKAGENAME" which will pick up long lines for example. I have a ruler/colored line at character 83 in vim, but I'm not yet ready with the setup to write correct code again in vim. There's a similar thing for emacs. I would replace the third or second "It" with @code{LMMS} or simply LMMS. =20 > > + (license license:gpl2+))) > > -- > > 2.11.1 >=20 > Ok, this is my first review, I tried to get it right but I probably for= got > something (I still can't get my own patches right on the first try :p). > Running "guix lint lmms" would have saved you the last comment ;). I ca= n't > try it now, but I'll test your patch (or an updated version) this eveni= ng. >=20