Re: [PATCH] gnu: Add lmms

[julien lepiller]

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 <address@hidden>
> 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.

> +    (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.