Bug 1455470

Summary:	Review Request: rubygem-gettext-setup - Easier internationalization with fast_gettext
Product:	[Fedora] Fedora	Reporter:	Dominic Cleal <dominic>
Component:	Package Review	Assignee:	Vít Ondruch <vondruch>
Status:	CLOSED NEXTRELEASE	QA Contact:	Fedora Extras Quality Assurance <extras-qa>
Severity:	medium	Docs Contact:
Priority:	medium
Version:	rawhide	CC:	package-review, vondruch
Target Milestone:	---	Flags:	vondruch: fedora-review+
Target Release:	---
Hardware:	All
OS:	Linux
Whiteboard:
Fixed In Version:	rubygem-gettext-setup-0.25-2.fc27	Doc Type:	If docs needed, set a value
Doc Text:		Story Points:	---
Clone Of:		Environment:
Last Closed:	2017-05-31 12:59:26 UTC	Type:	---
Regression:	---	Mount Type:	---
Documentation:	---	CRM:
Verified Versions:		Category:	---
oVirt Team:	---	RHEL 7.3 requirements from Atomic Host:
Cloudforms Team:	---	Target Upstream Version:
Embargoed:
Bug Depends On:
Bug Blocks:	1339959

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 1 Dominic Cleal 2017-05-31 08:06:27 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.25-1.fc27.src.rpm

Updated to 0.25 to drop the patch carried in the first version.

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.