Bug 839649 - Review Request: rubygem-rails_best_practices - a code metric tool for rails codes, written in Ruby.
Summary: Review Request: rubygem-rails_best_practices - a code metric tool for rails c...
Keywords:
Status: CLOSED NOTABUG
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: FE-DEADREVIEW
TreeView+ depends on / blocked
 
Reported: 2012-07-12 13:40 UTC by Maros Zatko
Modified: 2016-01-04 08:57 UTC (History)
6 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2016-01-04 08:57:33 UTC


Attachments (Terms of Use)

Description Maros Zatko 2012-07-12 13:40:02 UTC
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-1.fc17.src.rpm
Description: a code metric tool for rails codes, written in Ruby.
Fedora Account System Username: mzatko

Comment 1 Michal Fojtik 2012-07-12 14:20:37 UTC
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.

Comment 2 Maros Zatko 2012-09-03 15:36:40 UTC
Thanks for your review, I've updated spec, so

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-2.fc17.src.rpm
Description: a code metric tool for rails codes, written in Ruby.
Fedora Account System Username: mzatko

Comment 3 Maros Zatko 2012-09-04 08:58:56 UTC
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

Comment 4 Michal Fojtik 2012-09-04 09:00:51 UTC
Thanks, REVIEW+

Comment 5 Mo Morsi 2012-10-02 20:26:23 UTC
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.

Comment 7 Josef Stribny 2012-10-18 11:18:36 UTC
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

Comment 8 Josef Stribny 2012-12-10 16:54:24 UTC
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)

Comment 9 Vít Ondruch 2012-12-11 08:16:34 UTC
(In reply to comment #8)
> 5, Why do you list subfolders from
> %{gem_instdir}/spec/rails_best_practices/? Isn't enough to list it as one
> folder only?

I would put it differently. There is nothing wrong in listing subfolders. This may help to assure, that the specific folders are included in package. However, as it is done currently, only the files are owned by package, not the directories itself. And that is wrong. This applies not just to %{gem_instdir}/spec/ but to %{gem_instdir}/assets/ as well.

Comment 10 Vít Ondruch 2012-12-11 08:27:21 UTC
(In reply to comment #8)
> 2, Get rid of 
> 
> Requires: ruby

Agree with this.

> BuildRequires: ruby

Don't fully agree. It is safer option to include BR: ruby ATM.

Comment 11 Josef Stribny 2013-04-22 10:33:09 UTC
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

Comment 13 Vít Ondruch 2016-01-04 08:57:33 UTC
Closing this stalled review.


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