Bug 737511 - Review request: rubygem-haml-rails - Haml generators for Rails 3
Summary: Review request: rubygem-haml-rails - Haml generators for Rails 3
Keywords:
Status: CLOSED RAWHIDE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Vít Ondruch
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks: 705514
TreeView+ depends on / blocked
 
Reported: 2011-09-12 11:34 UTC by Bohuslav "Slavek" Kabrda
Modified: 2011-09-23 08:50 UTC (History)
3 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2011-09-23 08:50:45 UTC
vondruch: fedora-review+
gwync: fedora-cvs+


Attachments (Terms of Use)

Description Bohuslav "Slavek" Kabrda 2011-09-12 11:34:16 UTC
SPEC: http://bkabrda.fedorapeople.org/haml-rails/rubygem-haml-rails.spec
SRPM: http://bkabrda.fedorapeople.org/haml-rails/rubygem-haml-rails-0.3.4-1.fc15.src.rpm

Description:
Haml-rails provides Haml generators for Rails 3. It also enables Haml as the
templating engine for you, so you don't have to screw around in your own
application.rb when your Gemfile already clearly indicated what templating
engine you have installed. Hurrah.

Koji:
http://koji.fedoraproject.org/koji/taskinfo?taskID=3344497

Comment 1 Vít Ondruch 2011-09-12 11:36:32 UTC
I'll take this one.

Comment 2 Vít Ondruch 2011-09-12 14:03:48 UTC
* Test suite execution
  - I would suggest to replace the %check section with following content which
    should provide similar results:

    pushd .%{geminstdir}
    testrb -Itest test/lib/generators/haml/*_test.rb
    popd

* Provides differs from upstream
  - Is there any reason why you require rubygem(rails) instead of the gem set
    which is used by upstream? Requires should be kept minimal as possible. 
    The test suite passes if rubygem(rails) dependency is replaced by:

    BuildRequires: rubygem(actionmailer)
    BuildRequires: rubygem(activerecord)

    Are these gems really hard requirement? It seems that they are optional.
    Please investigate.

* Keep the test files
  - If the test suite is part of the gem, please keep the tests in -doc
    subpackage.
  - The Gemfile should be also kept in -doc subpackage, as it is intended to be
    used by the test suite

Comment 3 Bohuslav "Slavek" Kabrda 2011-09-13 07:16:45 UTC
(In reply to comment #2)
> * Test suite execution
>   - I would suggest to replace the %check section with following content which
>     should provide similar results:
> 
>     pushd .%{geminstdir}
>     testrb -Itest test/lib/generators/haml/*_test.rb
>     popd
> 

- Done

> * Provides differs from upstream
>   - Is there any reason why you require rubygem(rails) instead of the gem set
>     which is used by upstream? Requires should be kept minimal as possible. 
>     The test suite passes if rubygem(rails) dependency is replaced by:
> 
>     BuildRequires: rubygem(actionmailer)
>     BuildRequires: rubygem(activerecord)
> 
>     Are these gems really hard requirement? It seems that they are optional.
>     Please investigate.
>

- Yes, the tests pass without rubygem(rails) and with these two included, but you also need to include BR: rubygem(railties)
 
> * Keep the test files
>   - If the test suite is part of the gem, please keep the tests in -doc
>     subpackage.
>   - The Gemfile should be also kept in -doc subpackage, as it is intended to be
>     used by the test suite

- Done

SPEC: http://bkabrda.fedorapeople.org/haml-rails/rubygem-haml-rails.spec
SRPM: http://bkabrda.fedorapeople.org/haml-rails/rubygem-haml-rails-0.3.4-2.fc15.src.rpm
Koji: http://koji.fedoraproject.org/koji/taskinfo?taskID=3347553

Comment 4 Vít Ondruch 2011-09-13 08:10:24 UTC
* %doc macro in -doc sub-package
  - I would suggest to mark by %doc macro only documents. It means that the test
    suite and the Gemfile should not be marked as doc.
  - There are also other opinions, such as that everything in -doc sub-package
    is documentation and there is no need to mark anything as a documentation.
    However I do not share this POV.

* Substantially different R and BR
  - Requires and BuildRequires differ substantially, so I wonder what is the
    reason for the difference. I guess that ActiveRecord and ActionMailer
    are just optional dependencies, but I'd like to be sure.

Comment 5 Bohuslav "Slavek" Kabrda 2011-09-13 09:58:04 UTC
(In reply to comment #4)
> * %doc macro in -doc sub-package
>   - I would suggest to mark by %doc macro only documents. It means that the
> test
>     suite and the Gemfile should not be marked as doc.
>   - There are also other opinions, such as that everything in -doc sub-package
>     is documentation and there is no need to mark anything as a documentation.
>     However I do not share this POV.
>
I definitelly agree, that marking tests and Gemfile as doc wasn't appropriate - fixed.

> * Substantially different R and BR
>   - Requires and BuildRequires differ substantially, so I wonder what is the
>     reason for the difference. I guess that ActiveRecord and ActionMailer
>     are just optional dependencies, but I'd like to be sure.

ActiveRecord and ActionMailer are really required during tests, but I managed to remove some Requires (concretely activesupport and actionpack), which are already required by railties.

SPEC: http://bkabrda.fedorapeople.org/haml-rails/rubygem-haml-rails.spec
SRPM: http://bkabrda.fedorapeople.org/haml-rails/rubygem-haml-rails-0.3.4-3.fc15.src.rpm
Koji: http://koji.fedoraproject.org/koji/taskinfo?taskID=3347929

Comment 6 Bohuslav "Slavek" Kabrda 2011-09-14 07:36:57 UTC
Additional info:
- controller generator can be used to generate a controller without actionmailer and activerecord
- scaffold generator can be used with other ORM (tested with datamapper)
- mailer generator always generates a subclass of ActionMailer::Base, so it seems that for its proper usage, ActionMailer is required

Summary: I would recommend adding Requires: rubygem(actionmailer) even though haml-rails can work with controllers and scaffolds even without it.

Comment 7 Vít Ondruch 2011-09-20 07:06:33 UTC
* I believe that not every Rails application has to use ActionMailer for sending
  emails, so the actionmailer is just optional from haml-rails POV.

* Remove unused "testdir" macro

* %{geminstdir}/haml-rails.gemspec should not be marked as %doc

Otherwise the package looks good. So please fix this nits and go ahead with the Git request. This package is APPROVED.

Comment 8 Bohuslav "Slavek" Kabrda 2011-09-20 07:48:14 UTC
New Package SCM Request
=======================
Package Name: rubygem-haml-rails
Short Description: Haml generators for Rails 3
Owners: bkabrda
Branches: 
InitialCC:

Comment 9 Gwyn Ciesla 2011-09-20 14:55:54 UTC
Git done (by process-git-requests).

Comment 10 Vít Ondruch 2011-09-23 08:50:45 UTC
Already available in Rawhide.


Note You need to log in before you can comment on or make changes to this bug.