[Bug 640627] Review Request: rubygem-factory_girl - Framework and DSL for defining and using model instance factories

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

Mohammed Morsi <mmorsi at redhat.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |mmorsi at redhat.com

--- Comment #1 from Mohammed Morsi <mmorsi at redhat.com> 2010-10-12 22:20:05 EDT ---
Will take this one. Overall looks good, some specific comments though

* Can you change source0 to point to the official gem hosted at
http://rubygems.org/downloads/%{name}-%{version}.gem

* The LICENSE file should be part of the main package and marked as %doc. This
is so that if only the main package is installed and not the docs subpackage
(as usually is the case) the LICENSE is still included. 

http://fedoraproject.org/wiki/Packaging/LicensingGuidelines#License_Text

* Are sqlite3-ruby & activerecord really Requires or just BuildRequires (for
tests)? Looking at the factory_girl source it seems like they are only pulled
in for testing purposes.

* Can you fix the Rakefile with a patch in the spec (eg %patch0) instead of the
hotfix in %install section.

* The %files section for both packages need default attributes:
 %defattr(-, root, root, -)

Thanks for this submission.

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