Bug 1460662

Summary:	Review Request: rubygem-mustermann - A library using patterns like regular expressions
Product:	[Fedora] Fedora	Reporter:	Jun Aruga <jaruga>
Component:	Package Review	Assignee:	Vít Ondruch <vondruch>
Status:	CLOSED RAWHIDE	QA Contact:	Fedora Extras Quality Assurance <extras-qa>
Severity:	unspecified	Docs Contact:
Priority:	unspecified
Version:	rawhide	CC:	package-review, vondruch
Target Milestone:	---	Flags:	vondruch: fedora-review+
Target Release:	---
Hardware:	Unspecified
OS:	Unspecified
Whiteboard:
Fixed In Version:	rubygem-mustermann-1.0.0-1.fc27	Doc Type:	If docs needed, set a value
Doc Text:		Story Points:	---
Clone Of:		Environment:
Last Closed:	2017-06-13 16:47:10 UTC	Type:	Bug
Regression:	---	Mount Type:	---
Documentation:	---	CRM:
Verified Versions:		Category:	---
oVirt Team:	---	RHEL 7.3 requirements from Atomic Host:
Cloudforms Team:	---	Target Upstream Version:
Embargoed:

Description Jun Aruga 2017-06-12 11:06:23 UTC

Spec URL: https://pagure.io/jaruga_rubygem-mustermann/raw/master/f/rubygem-mustermann.spec
SRPM URL: https://pagure.io/jaruga_rubygem-mustermann/raw/master/f/rubygem-mustermann-1.0.0-1.fc27.src.rpm
Description: A library using patterns like regular expressions
Fedora Account System Username: jaruga

Koji: https://koji.fedoraproject.org/koji/taskinfo?taskID=19993792

Comment 1 Vít Ondruch 2017-06-13 06:21:45 UTC

I'll take it for review ...

Comment 2 Vít Ondruch 2017-06-13 07:32:54 UTC

* Summary/Description
  - I'd say that the summary is misleading. The original (but capitalized) would
    be better IMHO.
  - The description should start with capital letter.

* Group tag is deprecated
  - Please remove the Group tag. It is deprecated [1].

* Test {support,mustermann-contrib} libraries
  - It would be good to document why the {support,mustermann-contrib} is needed
    (e.g. "Support routines required by test suite").
  - I don't think there is any reason to expand these libraries into the
    .%{gem_instdir}. Although we are using this on several places, this is used
    typically to mimic the upstream repository layout. In this case, I think it
    would be much better to expand the sources using the %setup macro in %prep
    section, e.g.:

~~~
%setup -q -D -T -n  %{gem_name}-%{version} -b1 -b2
~~~

* Idiomatic test suite execution.
  - We typically execute the RSpec test suite via "rspec spec". If you need to
    specify additional path, the "rspec" command supports -I variable just as
    ruby does. E.g. this should work for you:

~~~
rspec -I{support,mustermann-contrib}/lib spec
~~~



[1] https://fedoraproject.org/wiki/Packaging:Guidelines#Tags_and_Sections

Comment 3 Vít Ondruch 2017-06-13 07:35:04 UTC

BTW there appears to be circular dependency with Sinatra, you should consider to prepare Mustermann for bootstrap.

Comment 4 Jun Aruga 2017-06-13 10:15:34 UTC

Vit, thank you for the review.

I fixed those items that you mentioned.


1 point.

> ~~~
> %setup -q -D -T -n  %{gem_name}-%{version} -b1 -b2
> ~~~

Below "-b n" looks better regarding the official document' sample.

~~~
%setup -q -D -T -n  %{gem_name}-%{version} -b 1 -b 2
~~~

http://ftp.rpm.org/max-rpm/s1-rpm-inside-macros.html
> %setup -T -b 0


I updated below files.

Spec URL: https://pagure.io/jaruga_rubygem-mustermann/raw/master/f/rubygem-mustermann.spec
SRPM URL: https://pagure.io/jaruga_rubygem-mustermann/raw/master/f/rubygem-mustermann-1.0.0-1.fc27.src.rpm


Could you check again?
Thanks.

Comment 5 Vít Ondruch 2017-06-13 11:13:16 UTC

Just one minor nit. I'd move the seds out of the pushd/popd block, since they are using absolute path now.

Otherwise the package looks good => APPROVED.

Comment 6 Gwyn Ciesla 2017-06-13 15:43:51 UTC

Package request has been approved: https://admin.fedoraproject.org/pkgdb/package/rpms/rubygem-mustermann

Comment 7 Jun Aruga 2017-06-13 16:47:10 UTC

Thank you. I pushed the files including your additional mentioning item.