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 <thylak...@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.
>
