https://bugzilla.redhat.com/show_bug.cgi?id=839649 Michal Fojtik <mfojtik(a)redhat.com&gt; changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |mfojtik(a)redhat.com --- Comment #1 from Michal Fojtik <mfojtik(a)redhat.com&gt; --- Review: - Summary && %description should start with the capital 'A' - No tests Please check if those tests are not included in gem, if so, I would ask upstream to add them. But not a review-blocker: https://github.com/railsbp/rails_best_practices/tree/master/spec - The list of files in the %files section looks a bit ugly, it is possible to somehow compress it? Like using directories rather than listing all files? - Please exclude the '.yardoc/*' files, then are not needed for the gem - Also consider removing this files: %{gem_instdir}/.gemtest %{gem_instdir}/.gitignore %{gem_instdir}/.rspec %{gem_instdir}/.rvmrc %{gem_instdir}/.travis.yml %{gem_instdir}/Gemfile %{gem_instdir}/Gemfile.lock %{gem_instdir}/Guardfile - The MIT_LICENSE should go into main %files section - I also think the 'License: GPLv2+ or Ruby' is wrong, since the license seems to be MIT. -- You are receiving this mail because: You are on the CC list for the bug.

https://bugzilla.redhat.com/show_bug.cgi?id=839649 --- Comment #3 from Maros Zatko <mzatko(a)redhat.com&gt; --- Once again, with compacted file list Spec URL: http://v3.sk/~hexo/rpm/rails_best_practices.spec SRPM URL: http://v3.sk/~hexo/rpm/rubygem-rails_best_practices-1.10.1-3.fc17.src.rpm -- You are receiving this mail because: You are on the CC list for the bug.

https://bugzilla.redhat.com/show_bug.cgi?id=839649 Mo Morsi <mmorsi(a)redhat.com&gt; changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |mmorsi(a)redhat.com --- Comment #5 from Mo Morsi <mmorsi(a)redhat.com&gt; --- Couple post review-nits * the spec file should be named rubygem-rails_best_practices * can you run the spec suite in a check section in the specfile? * please move the spec suite into the docs subpackage * please rm the files you exclude earlier in the spec and remove those excludes from the files section * slim is listed as a dev dependency on rubygems.org but is not referenced in this spec, is it needed? Thanks. -- You are receiving this mail because: You are on the CC list for the bug.

https://bugzilla.redhat.com/show_bug.cgi?id=839649 Josef Stribny <jstribny(a)redhat.com&gt; changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |jstribny(a)redhat.com --- Comment #7 from Josef Stribny <jstribny(a)redhat.com&gt; --- Hi, I believe Summary should start with a capital "A" for consistency as in How to create an RPM package guide [1] and as Michal Fojtik already suggested. (This is an informal review.) [1] https://fedoraproject.org/wiki/How_to_create_an_RPM_package -- You are receiving this mail because: You are on the CC list for the bug.

Product: Fedora https://bugzilla.redhat.com/show_bug.cgi?id=839649 --- Comment #8 from Josef Stribny <jstribny(a)redhat.com&gt; --- Because it is still unresolved, I am taking this to continue with the review. 1, Summary should start with a capital "A" as mentioned above 2, Get rid of Requires: ruby BuildRequires: ruby Requires: ruby(api) will require this for you and you may use it with other interpretations of Ruby. 3, Mark %{gem_instdir}/README.md and %{gem_instdir}/MIT_LICENSE as %doc 4, Move %{gem_instdir}/rails_best_practices.gemspec and %{gem_instdir}/Gemfile to doc subpackage 5, Why do you list subfolders from %{gem_instdir}/spec/rails_best_practices/? Isn't enough to list it as one folder only? 6, Consider upgrading it to the latest upstream version (1.13.1) -- You are receiving this mail because: You are on the CC list for the bug. Unsubscribe from this bug https://bugzilla.redhat.com/token.cgi?t=p73aQlq63O&a=cc_unsubscribe

Product: Fedora https://bugzilla.redhat.com/show_bug.cgi?id=839649 --- Comment #10 from Vít Ondruch <vondruch(a)redhat.com&gt; --- (In reply to comment #8) Agree with this. Don't fully agree. It is safer option to include BR: ruby ATM. -- You are receiving this mail because: You are on the CC list for the bug. Unsubscribe from this bug https://bugzilla.redhat.com/token.cgi?t=lRWw623IXg&a=cc_unsubscribe

Product: Fedora https://bugzilla.redhat.com/show_bug.cgi?id=839649 --- Comment #11 from Josef Stribny <jstribny(a)redhat.com&gt; --- Maros, are you still working on this? If so, please notice the change in the guidelines regarding Fedora 19 [1] and make necessary changes to the spec file. 1, Please change: - Requires: ruby(abi) = %{rubyabi} to Requires: ruby(release) - BuildRequires: ruby(abi) = %{rubyabi} to BuildRequires: ruby(release) 2, Use %gem_install macro instead of calling gem install directly [1] https://fedoraproject.org/wiki/Packaging:Ruby -- You are receiving this mail because: You are on the CC list for the bug. Unsubscribe from this bug https://bugzilla.redhat.com/token.cgi?t=B7oJmY8ZLj&a=cc_unsubscribe

https://bugzilla.redhat.com/show_bug.cgi?id=839649 John Skeoch <jskeoch(a)redhat.com&gt; changed: What |Removed |Added ---------------------------------------------------------------------------- Assignee|jstribny(a)redhat.com |hhorak(a)redhat.com -- You are receiving this mail because: You are on the CC list for the bug. You are always notified about changes to this product and component

https://bugzilla.redhat.com/show_bug.cgi?id=839649 Vít Ondruch <vondruch(a)redhat.com&gt; changed: What |Removed |Added ---------------------------------------------------------------------------- Assignee|hhorak(a)redhat.com |vondruch(a)redhat.com -- You are receiving this mail because: You are on the CC list for the bug. You are always notified about changes to this product and component