Re: [PATCH 1/1] gnu: Add dvtm.

[Leo Famulari]

Hello,

Thanks for your detailed critique of my first patch!

On Fri, Aug 21, 2015, at 18:35, Mark H Weaver wrote:
> Please include the Makefile variable being changed, like this:
> 
> * gnu-system.am (GNU_SYSTEM_MODULES): Add it.

Done.

> This new file needs the same header as our other source files, but with
> only your own copyright notice on top.

Done. 
 
> Please don't use any tabs in *.scm files.

All the tabs are gone.

I'm new to scheme so I tried to follow Riastradh's Lisp Style Rules, as
suggested in guix docs section 7.4.4 (Coding Style). Basically, I just
used C-M-q in vanilla emacs but I guess that that is not enough. I will
have to read more code and configure my editor more carefully...

> Please keep all lines in *.scm files to 80 columns or less.  Also, there
> should be a space after "version".

Done.

> "(arguments" should be lined up with "(build-system", and the "`" should
> be below the 'a'.  

Done.
 
> > +         #:phases (alist-replace 'configure (lambda _ (setenv "CC" (which 
> > "gcc"))) %standard-phases)
> 
> You should be able to remove this line and simply add "CC=gcc" to the
> make flags.

I'm glad you told me this. That line was copy-pasted. I've replaced the
part that skipped configure and check using modify-phases, which I find
much clearer.

> Please add a comment saying why tests are disabled.  It could be as
> simple as:
> 
>       #:tests? #f))  ; no test suite

Done.

> > +   (description "dvtm brings the concept of tiling window management, 
> > +popularized by X11-window managers like dwm to the console.  As a console 
> > window 
> > +manager it tries to make it easy to work with multiple console based 
> > programs.  ")
> 
> There should probably be a comma after "dwm" here.  Please remove the
> extraneous spaces at the end of the description, and wrap to fit in 80
> columns.

I copied this description verbatim from the project's home page. Think I
should file a bug upstream? ;)

I hadn't noticed the error but now it bugs me, so I've made the change.

Trailing whitespace removed.

> > +   (home-page "http://www.brain-dump.org/projects/dvtm/";)
> > +   (license (list isc x11))))
> 
> When there is more than one license, we add a comment above the
> 'license' field explaining the license situation.  We need to do this
> because the meaning of our license lists is ambiguous: it might mean
> dual-licensing, or it might mean that some of the code is under one
> license and some is under another.
> 
> Also, it should be the 'expat' license, not the 'x11' license.  They are
> both often ambiguously called the "mit" license, and are very similar,
> so it's a common mistake.

Done.

> Can you send an updated patch?

It follows this email.

Thanks for your help. Please let know if there are problems with the new
revision.

Leo


On Fri, Aug 21, 2015, at 18:35, Mark H Weaver wrote:
> Hi!
> 
> Leo Famulari <address@hidden> writes:
> > * gnu/packages/dvtm.scm: New file.
> > * gnu-system.am: Add it.
> 
> Please include the Makefile variable being changed, like this:
> 
> * gnu-system.am (GNU_SYSTEM_MODULES): Add it.
> 
> > diff --git a/gnu/packages/dvtm.scm b/gnu/packages/dvtm.scm
> > new file mode 100644
> > index 0000000..83ee293
> > --- /dev/null
> > +++ b/gnu/packages/dvtm.scm
> > @@ -0,0 +1,29 @@
> > +(define-module (gnu packages dvtm)
> 
> This new file needs the same header as our other source files, but with
> only your own copyright notice on top.
> 
> > +  #:use-module (guix packages)
> > +  #:use-module (guix download)
> > +  #:use-module (guix build-system gnu)
> > +  #:use-module (guix licenses)
> > +  #:use-module (gnu packages ncurses))
> > +
> > +(define-public dvtm 
> > +  (package
> > +   (name "dvtm")
> > +   (version "0.14")
> > +   (source (origin
> > +       (method url-fetch)
> 
> Please don't use any tabs in *.scm files.
> 
> > +       (uri (string-append "http://www.brain-dump.org/projects/dvtm/dvtm-"; 
> > version".tar.gz"))
> 
> Please keep all lines in *.scm files to 80 columns or less.  Also, there
> should be a space after "version".
> 
> > +       (sha256
> > +        (base32 "0ykl8dz7ivjgdzhmhlgidnp2ffh5gxq9lbg276w7iid4z10v76wa"))))
> > +   (build-system gnu-build-system)
> > +     (arguments
> > +       `(#:make-flags (list (string-append "PREFIX=" (assoc-ref %outputs 
> > "out")))
> 
> "(arguments" should be lined up with "(build-system", and the "`" should
> be below the 'a'.  
> 
> > +         #:phases (alist-replace 'configure (lambda _ (setenv "CC" (which 
> > "gcc"))) %standard-phases)
> 
> You should be able to remove this line and simply add "CC=gcc" to the
> make flags.
> 
> > +         #:tests? #f))
> 
> Please add a comment saying why tests are disabled.  It could be as
> simple as:
> 
>       #:tests? #f))  ; no test suite
> 
> > +
> > +   (inputs `(("ncurses" ,ncurses)))
> > +   (synopsis "Tiling window management for the console")
> > +   (description "dvtm brings the concept of tiling window management, 
> > +popularized by X11-window managers like dwm to the console.  As a console 
> > window 
> > +manager it tries to make it easy to work with multiple console based 
> > programs.  ")
> 
> There should probably be a comma after "dwm" here.  Please remove the
> extraneous spaces at the end of the description, and wrap to fit in 80
> columns.
> 
> > +   (home-page "http://www.brain-dump.org/projects/dvtm/";)
> > +   (license (list isc x11))))
> 
> When there is more than one license, we add a comment above the
> 'license' field explaining the license situation.  We need to do this
> because the meaning of our license lists is ambiguous: it might mean
> dual-licensing, or it might mean that some of the code is under one
> license and some is under another.
> 
> Also, it should be the 'expat' license, not the 'x11' license.  They are
> both often ambiguously called the "mit" license, and are very similar,
> so it's a common mistake.
> 
> Can you send an updated patch?
> 
>      Thanks,
>        Mark