Bug 1460662 - Review Request: rubygem-mustermann - A library using patterns like regular expressions
Summary: Review Request: rubygem-mustermann - A library using patterns like regular ex...
Keywords:
Status: CLOSED RAWHIDE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: Unspecified
OS: Unspecified
unspecified
unspecified
Target Milestone: ---
Assignee: Vít Ondruch
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2017-06-12 11:06 UTC by Jun Aruga
Modified: 2017-06-13 16:47 UTC (History)
2 users (show)

Fixed In Version: rubygem-mustermann-1.0.0-1.fc27
Doc Type: If docs needed, set a value
Doc Text:
Clone Of:
Environment:
Last Closed: 2017-06-13 16:47:10 UTC
Type: Bug
Embargoed:
vondruch: fedora-review+


Attachments (Terms of Use)

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.


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