Re: [PATCH] gnu: Add lmms

[ng0 <contact.ng0@cryptolab.net>]

On 17-02-23 10:15:47, julien lepiller wrote:
> Le 2017-02-23 00:18, Rodger Fox a écrit :
> > Here is another package. I think I got the commit message right this
> > time. Let me know if I missed anything.
> 
> Hi!
> 
> I wanted to try this software for while now, so you're great :)
> 
> I added some comments on your patch below:
> 
> > From 8e2757bee584f4686e02da512662fb512b05c037 Mon Sep 17 00:00:00 2001
> > From: Rodger Fox <thylakoid@openmailbox.org>
> > 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.
> 
> > +       (sha256
> > +        (base32
> > +	 "1g76z7ha3hd53vbqaq9n1qg6s3lw8zzaw51iny6y2bz0j1xqwcsr"))))
> There's a tab in the indentation, please use spaces.
> 
> > +    (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:
> 
> (arguments
>  `(#:tests? #f
>    #:validate-runpath? #f))
> 
> Why do you need to disable runpath validation?
> 
> > +    (native-inputs
> > +     `(("pkg-config" ,pkg-config)))
> > +     (inputs
> Indentation is off by one (inputs should be aligned with native-inputs).
> 
> > +    (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 soft
> > 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.
  
> > +    (license license:gpl2+)))
> > --
> > 2.11.1
> 
> Ok, this is my first review, I tried to get it right but I probably forgot
> 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 can't
> try it now, but I'll test your patch (or an updated version) this evening.
>