Bug 1455470 - Review Request: rubygem-gettext-setup - Easier internationalization with fast_gettext
Summary: Review Request: rubygem-gettext-setup - Easier internationalization with fast...
Status: CLOSED NEXTRELEASE
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:
Keywords:
Depends On:
Blocks: 1339959
TreeView+ depends on / blocked
 
Reported: 2017-05-25 09:18 UTC by Dominic Cleal
Modified: 2017-05-31 12:59 UTC (History)
2 users (show)

(edit)
Clone Of:
(edit)
Last Closed: 2017-05-31 12:59:26 UTC
vondruch: fedora-review+


Attachments (Terms of Use)

Description Dominic Cleal 2017-05-25 09:18:25 UTC
Spec URL: https://fedorapeople.org/~domcleal/reviews/rubygem-gettext-setup/rubygem-gettext-setup.spec
SRPM URL: https://fedorapeople.org/~domcleal/reviews/rubygem-gettext-setup/rubygem-gettext-setup-0.24-1.fc27.src.rpm
Description: A gem to ease internationalization with fast_gettext, used by Puppet and related projects
Fedora Account System Username: domcleal

Comment 2 Vít Ondruch 2017-05-31 08:36:52 UTC
I'll take a look ...

Comment 3 Vít Ondruch 2017-05-31 10:31:40 UTC
* Group is not needed anymore
  - The Group tag should not be used for F26+ [1].

* Unnecessary build dependencies
  - If I am not mistaken, rubygem(rspec) will pull in all other rspec-*
    packages. Are these BR necessary?
  - I am not sure about webmock and rack-test. I removed the dependency,
    searched for some requires but found nothing. Are they needed?
  - I don't think you need simplecov for anything, since we don't really care
    about code coverage. I'd get rid of the dependency by st. like:

~~~
sed -i "/require 'simplecov'/ s/^/#/" spec/spec_helper.rb
sed -i "/SimpleCov.start/,/^end$/ s/^/#/" spec/spec_helper.rb
~~~

* Use executable instead of package dependencies.
  - This is jut minor nit and I'' leave it up to you, since it is more or less
    about style, but if the package requires some executables, such as 'git'
    or 'msgcmp' in this case, I prefer to require directly these executables,
    instead of the packages. E.g.:

~~~
BuildRequires: %{_bindir}/msgcmp
BuildRequires: %{_bindir}/git
~~~

Otherwise, the package looks good => APPROVE


[1] https://fedoraproject.org/w/index.php?title=Packaging:Guidelines&diff=487247&oldid=487238

Comment 4 Dominic Cleal 2017-05-31 11:10:06 UTC
Thanks very much for the review Vít, I agree with all of your comments. I updated the spec file and will request the package now.

Spec URL: https://fedorapeople.org/~domcleal/reviews/rubygem-gettext-setup/rubygem-gettext-setup.spec
SRPM URL: https://fedorapeople.org/~domcleal/reviews/rubygem-gettext-setup/rubygem-gettext-setup-0.25-2.fc27.src.rpm

Comment 5 Vít Ondruch 2017-05-31 12:04:29 UTC
(In reply to Dominic Cleal from comment #4)
> I updated the spec file

LGTM. Thx.

Comment 6 Gwyn Ciesla 2017-05-31 12:39:12 UTC
Package request has been approved: https://admin.fedoraproject.org/pkgdb/package/rpms/rubygem-gettext-setup

Comment 7 Dominic Cleal 2017-05-31 12:59:26 UTC
rubygem-gettext-setup-0.25-2.fc27 built.


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