Re: [PATCH] Add 12 rubygems.

[Ricardo Wurmus]

Here’s the review for the other 11 patches, starting from the last:

> From 4ccc1fadcb67a0d296bedd51a7f73d911da4680d Mon Sep 17 00:00:00 2001
> From: Ben Woodcroft <address@hidden>
> Date: Tue, 29 Dec 2015 15:13:13 +1000
> Subject: [PATCH 12/12] gnu: Add ruby-ttfunk.

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

[...]

> +    (synopsis "Font Metrics Parser for the Prawn PDF generator")

s/Metrics Parser/metrics parser/

> +    (description
> +     "TTFunk is a TrueType font parser written in pure ruby.  It is used as
> +part of the Prawn PDF generator.")

s/ruby/Ruby/

> +    ;; From the README: "Matz's terms for Ruby, GPLv2, or GPLv3. See LICENSE
> +    ;; for details."
> +    (license (list license:gpl2 license:gpl3
> +                   (license:non-copyleft
> +                    "file://src/LICENSE"
> +                    "See LICENSE in the distribution.")))))

The license is actually just one of “(list license:gpl2 license:gpl3
license:ruby)”.  LICENSE just contains what appears to be the Ruby
license.

> From 8a7a728c843f05206599d777fdd8c8a68f26eea4 Mon Sep 17 00:00:00 2001
> From: Ben Woodcroft <address@hidden>
> Date: Tue, 29 Dec 2015 15:05:18 +1000
> Subject: [PATCH 11/12] gnu: Add ruby-ascii85.

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

[...]

> +    (synopsis "Encode and decode Adobe's ascii85 binary-to-text encoding")

How about

  “Encode and decode Ascii85 binary-to-text encoding”

> +    (description
> +     "This library provides methods to encode and decode Adobe's Ascii85
> +binary-to-text encoding.  The main modern use of Ascii85 is in Adobe's
> +PostScript and Portable Document Format file formats.")

Also here I’d remove “Adobe’s”.  Additionally, you could wrap “Portable
Document Format” in @dfn{}:

     "This library provides methods to encode and decode Ascii85
binary-to-text encoding.  The main modern use of Ascii85 is in
PostScript and @dfn{Portable Document Format} (PDF) file formats."

> +    (home-page
> +     "https://github.com/datawraith/ascii85gem";)

Please pull this on one line.


> From 6c0f6264d58f4d1815df209ab0231c4abc054f7a Mon Sep 17 00:00:00 2001
> From: Ben Woodcroft <address@hidden>
> Date: Tue, 29 Dec 2015 15:00:15 +1000
> Subject: [PATCH 10/12] gnu: Add ruby-afm.

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

[...]

> +    (synopsis "Read Adobe Font Metrics (afm) files")
> +    (description
> +     "@code{afm} is a library to read Adobe Font Metrics (afm) files and use
> +the data therein.")

How about

  "This library provides methods to read @dfn{Adobe Font Metrics} (afm)
   files and use the data therein."

> From 8641b00e5706f6d1fe486df457b75f91a68c5edc Mon Sep 17 00:00:00 2001
> From: Ben Woodcroft <address@hidden>
> Date: Tue, 29 Dec 2015 14:52:04 +1000
> Subject: [PATCH 09/12] gnu: Add ruby-rc4.

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

[...]

> +    (arguments
> +     `(#:phases
> +       (modify-phases %standard-phases
> +         (replace 'check
> +           (lambda _
> +             (and
> +              (zero? (system* "rspec" "spec/rc4_spec.rb"))))))))

There’s no need for ‘(and ...)’ here.

> +    (native-inputs
> +     `(("ruby-rspec" ,ruby-rspec-2)))
> +    (synopsis "Implementation of the RC4 algorithm")
> +    (description
> +     "RubyRC4 is a pure Ruby implementation of the RC4 algorithm.")
> +    ;; link on rubygems.org is dead, so use github URL.

I think we can do without this comment.

> From 1f13b94614029c3ca7cdc34dbbcac2d2c69e786c Mon Sep 17 00:00:00 2001
> From: Ben Woodcroft <address@hidden>
> Date: Tue, 29 Dec 2015 14:51:34 +1000
> Subject: [PATCH 08/12] gnu: Add ruby-hashery.

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

[...]

> +(define-public ruby-hashery
> +  (package
> +    (name "ruby-hashery")
> +    (version "2.1.1")
> +    (source
> +     (origin
> +       (method url-fetch)
> +       (uri (rubygems-uri "hashery" version))
> +       (sha256
> +        (base32
> +         "0xawbljsjarl9l7700bka672ixwznzwih4s9i38p1y9mp8hyx54g"))))
> +    (build-system ruby-build-system)
> +    (arguments
> +     `(#:phases
> +       (modify-phases %standard-phases
> +         (replace 'check
> +           (lambda _
> +             (and
> +              (zero? (system* "qed"))
> +              (zero? (system* "rubytest" "-Ilib" "-Itest" "test/"))))))))

I would prefer moving ‘(and’ and ‘(zero?’ on the same line and then
split the “rubytest” line.

> +    (native-inputs
> +     `(("ruby-rubytest-cli" ,ruby-rubytest-cli)
> +       ("ruby-qed" ,ruby-qed)
> +       ("ruby-lemon" ,ruby-lemon)))
> +    (synopsis "Hash-like classes with extra features")
> +    (description
> +     "The Hashery is a tight collection of Hash-like classes.  Included are
> +the auto-sorting Dictionary class, the efficient LRUHash, the flexible
> +OpenHash and the convenient KeyHash.  Nearly every class is a subclass of the
> +CRUDHash which defines a CRUD (Create, Read, Update and Delete) model on top
> +of Ruby's standard Hash making it possible to subclass and augment to fit any
> +specific use case.")

Could you please wrap all class names in @code{...}?  E.g.

     "The Hashery is a tight collection of @code{Hash}-like classes.
Included are the auto-sorting @code{Dictionary} class, the efficient
@code{LRUHash}, the flexible @code{OpenHash} and the convenient
@code{KeyHash}.  Nearly every class is a subclass of the @code{CRUDHash}
which defines a CRUD (Create, Read, Update and Delete) model on top of
Ruby's standard @code{Hash} making it possible to subclass and augment
to fit any specific use case."


> From 9d9bbcfbfa52744b2b174330164578f5129187d4 Mon Sep 17 00:00:00 2001
> From: Ben Woodcroft <address@hidden>
> Date: Tue, 29 Dec 2015 14:45:17 +1000
> Subject: [PATCH 07/12] gnu: Add ruby-rubytest-cli.

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

[...]

> +    (synopsis
> +     "Command-line interface for rubytest")

Please let them share one line.

> +    (description
> +     "Rubytest CLI is a command-line interface for running tests for
> +Rubytest-based test frameworks.  It provides the @code{rubytest} 
> executable.")
> +    (home-page
> +     "http://rubyworks.github.io/rubytest-cli";)

Same here.

> From 5edfa6742d7ee5a958bae3e66e978ff63f10c41a Mon Sep 17 00:00:00 2001
> From: Ben Woodcroft <address@hidden>
> Date: Tue, 29 Dec 2015 14:42:53 +1000
> Subject: [PATCH 06/12] gnu: Add ruby-lemon.

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

[...]

> +++ b/gnu/packages/ruby.scm
> @@ -3045,3 +3045,35 @@ like Test::Unit and requirement specifications systems 
> like Cucumber.")
>  for reuse by other test frameworks.")
>      (home-page "http://rubyworks.github.io/ae";)
>      (license license:bsd-2)))
> +
> +(define-public ruby-lemon
> +  (package
> +    (name "ruby-lemon")
> +    (version "0.9.1")
> +    (source
> +    (origin
> +      (method url-fetch)
> +      (uri (rubygems-uri "lemon" version))
> +      (sha256
> +        (base32
> +          "0gqhpgjavgpvx23rqpfqcv3d5bs8gc7lr9yvj8kxgp7mfbdc2jcm"))))

The indentation here looks wrong.  The paren of “(base32” should be
aligned with the “s” of “sha256”, and the opening quote of the string
should be aligned with the “b” of “base32”.

> +    (build-system ruby-build-system)
> +    (arguments
> +     `(#:phases
> +       (modify-phases %standard-phases
> +         (replace 'check
> +           (lambda _
> +             (zero? (system* "qed")))))))

Could this all be one line starting from “(replace” or at least
“(lambda”?

> +    (propagated-inputs
> +     `(("ruby-ae" ,ruby-ae)
> +       ("ruby-ansi" ,ruby-ansi)
> +       ("ruby-rubytest" ,ruby-rubytest)))
> +    (native-inputs
> +     `(("ruby-qed" ,ruby-qed)))
> +    (synopsis "Test framework that correlates code structure and test
> unit")

You could shorten the synopsis a bit by replacing “that correlates” with
“correlating”.

> +    (description
> +     "Lemon is a Unit Testing Framework that enforces highly formal
> +case-to-class and unit-to-method test construction.  This enforcement can 
> help
> +focus concern on individual units of behavior.")

Please lowercase “Unit Testing Framework”.

> From a6f0972e10e7fe4ede3082cc22b24c0af69f7ae9 Mon Sep 17 00:00:00 2001
> From: Ben Woodcroft <address@hidden>
> Date: Tue, 29 Dec 2015 14:37:45 +1000
> Subject: [PATCH 05/12] gnu: Add ruby-ae.

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

[...]

> +    (source
> +     (origin
> +       (method url-fetch)
> +       ;; fetch from github so tests are included ?

Not sure? :)

> +       (uri (string-append
> +             "https://github.com/rubyworks/ae/archive/";
> +             version ".tar.gz"))
> +       (file-name (string-append name "-" version ".tar.gz"))
> +       (sha256
> +        (base32
> +         "147jmkx54x7asy2d8m4dyrhhf4hdx4galpnhwzai030y3cdsfrrl"))))
> +    (build-system ruby-build-system)
> +    (arguments
> +     `(#:phases
> +       (modify-phases %standard-phases
> +         (replace 'check
> +           (lambda _
> +             (zero? (system* "qed")))))))

This line is short enough to merge it with the “(lambda _”.

> +    (propagated-inputs
> +     `(("ruby-ansi" ,ruby-ansi)))
> +    (native-inputs
> +     `(("ruby-qed" ,ruby-qed)))
> +    (synopsis "Assertions library")
> +    (description
> +     "Assertive Expressive (AE) is an assertions library specifically 
> designed
> +for reuse by other test frameworks.")

That’s not very descriptive, but I know it’s difficult to come up with
good descriptions when the upstream descriptions are so vague.  Oh
well.  Let’s leave it like this if you cannot come up with something a
little more expressive.

> From dcb24fdc106d5878c4a5ff24d2fda6e2987bb15d Mon Sep 17 00:00:00 2001
> From: Ben Woodcroft <address@hidden>
> Date: Tue, 29 Dec 2015 14:36:20 +1000
> Subject: [PATCH 04/12] gnu: Add ruby-qed.

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

[...]

> +    (arguments
> +     ;; disable testing to break the cycle qed, ansi, qed, among 
> others.Instead
> +     ;; simply test that the executable runs.

The first line of the comment looks awfully long.  There is no space
after the first sentence.

> +     `(#:phases
> +       (modify-phases %standard-phases
> +         (replace 'check
> +           (lambda _
> +             (zero? (system* "ruby" "-Ilib" "bin/qed"
> "--copyright")))))))

Why “--copyright”?  Are you trying to run qed to see if there are any
errors, because there is no test suite?  If so, maybe you could state
your intention in a comment.

> +    (propagated-inputs
> +     `(("ruby-ansi" ,ruby-ansi)
> +       ("ruby-brass" ,ruby-brass)))
> +    (synopsis "Test framework utilizing literate programming techniques")
> +    (description
> +     "QED (Quality Ensured Demonstrations) is a TDD (Test Driven
> +Development)/BDD (Behaviour Drive Development) framework utilizing Literate
> +Programming techniques.  QED sits somewhere between lower-level testing tools
> +like Test::Unit and requirement specifications systems like
> Cucumber.")

Maybe this is better:

     "@dfn{Quality Ensured Demonstrations} (QED) is a test framework for
@dfn{Test Driven Development} (TDD) and @dfn{Behaviour Driven
Development} (BDD) utilizing Literate Programming techniques.  QED sits
somewhere between lower-level testing tools like @code{Test::Unit} and
requirement specifications systems like Cucumber."

> From 356e1fbe6f87e0da6410760a2f177021e5b0302c Mon Sep 17 00:00:00 2001
> From: Ben Woodcroft <address@hidden>
> Date: Tue, 29 Dec 2015 14:16:42 +1000
> Subject: [PATCH 03/12] gnu: Add ruby-brass.

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

[...]

> +(define-public ruby-brass
> +  ;; download from git so test runner is included

But... we are still downloading from Rubygems.

> +  (package
> +    (name "ruby-brass")
> +    (version "1.2.1")
> +    (source
> +     (origin
> +       (method url-fetch)
> +       (uri (rubygems-uri "brass" version))
> +       (sha256
> +        (base32
> +         "154lp8rp1vmg60ri1j4cb8hqlw37z7bn575h899v8hzxwi11sxka"))))
> +    (build-system ruby-build-system)
> +    (arguments
> +     ;; disable tests to break the cycle brass, lemon, ae, qed, brass. 
> Instead
> +     ;; simply test that the library can be require'd.

Again, the comment looks a bit long; also add another space after the
first sentence and capitalise “Disable” at the beginning.

> From 81e6d9cb5470f85131e302759c3698d9c6295dd8 Mon Sep 17 00:00:00 2001
> From: Ben Woodcroft <address@hidden>
> Date: Tue, 29 Dec 2015 14:13:42 +1000
> Subject: [PATCH 02/12] gnu: Add ruby-rubytest.

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

[...]

> +    (arguments
> +     ;; disable testing to break the cycle rubytest, qed, brass, rubytest, as
> +     ;; well as the cycle rubytest, qed, ansi, rubytest. Instead simply test
> +     ;; that the library can be require'd.

Same as above: capitalisation and space after first sentence.

> +    (home-page
> +     "http://rubyworks.github.io/rubytest";)

On one line please.

And that’s it as I reviewed the first patch already.  Thanks a lot for
the patches!  Could you please send updated patches?

~~ Ricardo