This service will be undergoing maintenance at 00:00 UTC, 2017-10-23 It is expected to last about 30 minutes
Bug 1460662 - Review Request: rubygem-mustermann - A library using patterns like regular expressions
Review Request: rubygem-mustermann - A library using patterns like regular ex...
Status: CLOSED RAWHIDE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
Unspecified Unspecified
unspecified Severity unspecified
: ---
: ---
Assigned To: Vít Ondruch
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2017-06-12 07:06 EDT by Jun Aruga
Modified: 2017-06-13 12:47 EDT (History)
2 users (show)

See Also:
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 12:47:10 EDT
Type: Bug
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
vondruch: fedora‑review+


Attachments (Terms of Use)

  None (edit)
Description Jun Aruga 2017-06-12 07:06:23 EDT
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 02:21:45 EDT
I'll take it for review ...
Comment 2 Vít Ondruch 2017-06-13 03:32:54 EDT
* 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 03:35:04 EDT
BTW there appears to be circular dependency with Sinatra, you should consider to prepare Mustermann for bootstrap.
Comment 4 Jun Aruga 2017-06-13 06:15:34 EDT
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 07:13:16 EDT
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 11:43:51 EDT
Package request has been approved: https://admin.fedoraproject.org/pkgdb/package/rpms/rubygem-mustermann
Comment 7 Jun Aruga 2017-06-13 12:47:10 EDT
Thank you. I pushed the files including your additional mentioning item.

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