Re: [PATCH] gnu: Add ruby-nokogiri

[Ricardo Wurmus]

Hi Pjotr,

following are a couple of comments about the patch.  (I did not try it
myself.)

> From e2fd935a659cacde5cb74bef19406f056a262f79 Mon Sep 17 00:00:00 2001
> From: pjotrp <address@hidden>
> Date: Mon, 13 Jul 2015 14:56:40 +0200
> Subject: [PATCH] gnu: Add ruby-nokogiri

> * gnu/packages/ruby.scm (gnu-nokogiri): New variable

The name of the variable is ‘ruby-nokogiri’, not ‘gnu-nokogiri’.  There
also should be a period at the end.

> +(define-public ruby-nokogiri
> +  (package
> +    (name "ruby-nokogiri")
> +    (version "1.6.6.2")
> +    (source (origin
> +              (method url-fetch)
> +              (uri (string-append
> +                 "https://github.com/sparklemotion/nokogiri/archive/v";
> +                    version ".tar.gz"))
> +              (file-name (string-append name "-" version ".tar.gz"))
> +           (patches (map search-patch
> +                            (list "ruby-nokogiri-Rakefile.patch")))

‘(patches ...)’ should be aligned with the previous line.  ‘(list’
should be aligned with ‘search-patch’.  Since you only have one patch,
though, I think using ‘(map search-patch’ is odd.

> +    (arguments
> +     '(
> +       #:tests? #f  ;; test fails because nokogiri can only test with a 
> built extension (now part of install phase)

‘'(’ should not be on a line of its own.  The margin comment should only
have one semicolon and the text should be broken up to avoid long lines.

> +       #:gem-flags (list "--use-system-libraries" (string-append 
> "--with-xml2-include=" (assoc-ref %build-inputs "libxml2") "/include/libxml2" 
> ))

This is a very long line.  It can be broken up easily.

> +       #:phases (alist-replace
> +                 'build
> +                 (lambda _
> +                ;; calling rake gem 2x begets a gem
> +                (system* "rake" "gem")
> +                (zero? (system* "rake" "gem")))
> +               %standard-phases)))

The alignment here is off.  Also consider using the ‘modify-phases’
syntax instead.  Why exactly is “rake gem” called twice?  What is
different the second time around?

> +    (native-inputs
> +     `(("ruby-hoe" ,ruby-hoe)
> +       ("ruby-rake-compiler", ruby-rake-compiler)))
> +    (inputs
> +     `(("zlib" ,zlib)
> +       ("libxml2" ,libxml2)
> +       ("libxslt" ,libxslt)))
> +    (synopsis "Nokogiri (鋸) is an HTML, XML, SAX, and Reader parser")
> +    (description "Nokogiri parses and searches XML/HTML very quickly, and 
> also has correctly implemented CSS3 selector support as well as XPath 1.0 
> support.")

Please break apart the long description into multiple lines.  In Emacs
you can do this easily with ‘fill-paragraph’ (bound to ‘M-q’ by
default).

> +    (home-page "http://www.nokogiri.org/";)
> +    (license license:x11)))
> +

~~ Ricardo