[Bug 737511] New: Review request: rubygem-haml-rails - Haml generators for Rails 3

[Red Hat Bugzilla]

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

Summary: Review request: rubygem-haml-rails - Haml generators for Rails 3

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

Summary: Review request: rubygem-haml-rails - Haml generators
                    for Rails 3
           Product: Fedora
           Version: rawhide
          Platform: All
        OS/Version: Linux
            Status: NEW
          Severity: medium
          Priority: medium
         Component: Package Review
        AssignedTo: [e-mail]
        ReportedBy: [e-mail]
         QAContact: [e-mail]
                CC: [e-mail],
                    [e-mail]
    Classification: Fedora
      Story Points: ---
              Type: ---

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

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

[Bug 737511] Review request: rubygem-haml-rails - Haml generators for Rails 3

[Red Hat Bugzilla]

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=737511

Vít Ondruch  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |ASSIGNED
                 CC|                            |[e-mail]
         AssignedTo|[e-mail]    |[e-mail]

--- Comment #1 from Vít Ondruch  2011-09-12 07:36:32 EDT ---
I'll take this one.

[Bug 737511] Review request: rubygem-haml-rails - Haml generators for Rails 3

[Red Hat Bugzilla]

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=737511

Vít Ondruch  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
             Blocks|                            |705514

[Bug 737511] Review request: rubygem-haml-rails - Haml generators for Rails 3

[Red Hat Bugzilla]

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=737511

--- Comment #2 from Vít Ondruch  2011-09-12 10:03:48 EDT ---
* 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

[Bug 737511] Review request: rubygem-haml-rails - Haml generators for Rails 3

[Red Hat Bugzilla]

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=737511

--- Comment #3 from Bohuslav Kabrda  2011-09-13 03:16:45 EDT ---
(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.s...
Koji: http://koji.fedoraproject.org/koji/taskinfo?taskID=3347553

[Bug 737511] Review request: rubygem-haml-rails - Haml generators for Rails 3

[Red Hat Bugzilla]

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=737511

--- Comment #4 from Vít Ondruch  2011-09-13 04:10:24 EDT ---
* %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.

[Bug 737511] Review request: rubygem-haml-rails - Haml generators for Rails 3

[Red Hat Bugzilla]

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=737511

--- Comment #5 from Bohuslav Kabrda  2011-09-13 05:58:04 EDT ---
(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.s...
Koji: http://koji.fedoraproject.org/koji/taskinfo?taskID=3347929

[Bug 737511] Review request: rubygem-haml-rails - Haml generators for Rails 3

[Red Hat Bugzilla]

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=737511

--- Comment #6 from Bohuslav Kabrda  2011-09-14 03:36:57 EDT ---
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.

[Bug 737511] Review request: rubygem-haml-rails - Haml generators for Rails 3

[Red Hat Bugzilla]

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=737511

Vít Ondruch  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
               Flag|                            |fedora-review+

--- Comment #7 from Vít Ondruch  2011-09-20 03:06:33 EDT ---
* 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.

[Bug 737511] Review request: rubygem-haml-rails - Haml generators for Rails 3

[Red Hat Bugzilla]

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=737511

--- Comment #8 from Bohuslav Kabrda  2011-09-20 03:48:14 EDT ---
New Package SCM Request
=======================
Package Name: rubygem-haml-rails
Short Description: Haml generators for Rails 3
Owners: bkabrda
Branches: 
InitialCC:

[Bug 737511] Review request: rubygem-haml-rails - Haml generators for Rails 3

[Red Hat Bugzilla]

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=737511

Bohuslav Kabrda  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
               Flag|                            |fedora-cvs?

[Bug 737511] Review request: rubygem-haml-rails - Haml generators for Rails 3

[Red Hat Bugzilla]

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=737511

--- Comment #9 from Jon Ciesla  2011-09-20 10:55:54 EDT ---
Git done (by process-git-requests).

[Bug 737511] Review request: rubygem-haml-rails - Haml generators for Rails 3

[Red Hat Bugzilla]

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=737511

Vít Ondruch  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|ASSIGNED                    |CLOSED
         Resolution|                            |RAWHIDE
        Last Closed|                            |2011-09-23 04:50:45

--- Comment #10 from Vít Ondruch  2011-09-23 04:50:45 EDT ---
Already available in Rawhide.