[Bug 543549] Review Request: rubygem-haml - XHTML/XML templating engine

[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=543549





--- Comment #3 from Mamoru Tasaka <mtasaka at ioa.s.u-tokyo.ac.jp>  2009-12-03 13:23:10 EDT ---
Some notes:

* %define -> %global
  - Now Fedora prefers to use %global over %define.
   
https://fedoraproject.org/wiki/Packaging/Guidelines#.25global_preferred_over_.25define

* License
  - test/haml/spec/README.md is under WTFPL so the license tag
    should be "MIT and WTFPL".

* Requires
  - Please add the needed rubygem related dependency.
    For example, lib/haml/html.rb contains:
--------------------------------------------------------
    62  require 'hpricot'
--------------------------------------------------------
    So this package may need "Requires: rubygem(hpricot)" (here
    not speaking of BuildRequires).
    Note that I don't know if this dependency is optional or not.
    Also please check other dependency (if any).

* %check
  - I think
    * hardcoding test files as %test_files is not preferable.
      It is difficult to see what this file list came from.
    * also even if hardcoding these files is needed, defining %test_files
      is not needed because
      - %test_files is in essence used only in one place (in %check)
      - Whether adding executable permission to a script or not should
        be determined (for this case) by checking if the script has
        shebang or not, and should not be determined by hardcoded file list.

    I think
    - fixing Rakefile and execute "rake test"
    - or using
------------------------------------------------------------
pushd %{buildroot}%{geminstdir}
# The following -path list is from Rakefile
find * \
 -path 'test/*/*_test.rb' \
 -not -path 'test/rails/*' \
 -not -path 'test/plugins/*' \
 -not -path 'test/haml/spec/*' | \
 while read f
do
 ruby $f
done
------------------------------------------------------------
    is better.

* Macros
  - As %geminstdir is already defined, use the macro in %files.

* %changelog style
  - When using Fedora CVS system, it is convenient when you put one line
    between each %changelog entry (for "make clog", for example), like
------------------------------------------------------------
%changelog
* Wed Dec 02 2009 Michal Babej <mbabej at redhat.com> - 2.2.15-1
- Update to new upstream release

* Wed Dec 02 2009 Michal Babej <mbabej at redhat.com> - 2.2.14-1
- Initial package
-------------------------------------------------------------

By the way it is appreciated if you post the full URL of the new
spec/srpm.

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