[Bug 542559] Review Request: rubygem-thor - Scripting framework that replaces rake, sake and rubigen

[bugzilla at redhat.com]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.


https://bugzilla.redhat.com/show_bug.cgi?id=542559





--- Comment #2 from Matthew Kent <mkent at magoazul.com>  2009-12-21 00:12:21 EDT ---
Thank you for the review.

(In reply to comment #1)
> Some notes:
> 
> * Source
>   - Source0 in srpm differs from what I could download from
>     the URL written in the spec file
> -------------------------------------------------------------
> 5fa79d6ca562a39c72c89f5447a3fbd5  thor-0.12.0.gem
> 32e034949be3726ff1857d0edeae6566 
> rubygem-thor-0.12.0-1.fc13.src/thor-0.12.0.gem
> -------------------------------------------------------------
>     (and the contents of two gems actually differ)
> 

Good catch! Looks like they replaced it after the recent rubyforge -> gemcutter
migration.

> * Requires
>   - bin/rake2thor contains:
> -------------------------------------------------------------
>      8  require 'rake'
> -------------------------------------------------------------
>     As the Summary of this spec file says "that replaces rake,",
>     I think it is admitted to add "Requires: rubygem(rake)" and
>     this should surely be added.
> 

Yeah I went back and forth on this one a bit initial, but your right, it should
be there.

>   - lib/thor/shell/color.rb contains:
> -------------------------------------------------------------
>     98            @diff_lcs_loaded = begin
>     99              require 'diff/lcs'
>    100              true
>    101            rescue LoadError
>    102              false
>    103            end
> -------------------------------------------------------------
>     I guess "diff/lcs" dependency is surely optional, however
>     as Fedora already has rubygem-diff-lcs, you may want to
>     add this dependency (however this is up to what you think)  

Oh, missed this one. Looks like a minimal impact to add it as diff-lcs doesn't
have any dependencies of its own.

-- 
Configure bugmail: https://bugzilla.redhat.com/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are on the CC list for the bug.