[Bug 717645] New: Review Request: rubygem-compass-960-plugin - Compass compatible Sass port of 960.gs

[Red Hat Bugzilla]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.

Summary: Review Request: rubygem-compass-960-plugin - Compass compatible Sass port of 960.gs

https://bugzilla.redhat.com/show_bug.cgi?id=717645

Summary: Review Request: rubygem-compass-960-plugin - Compass
                    compatible Sass port of 960.gs
           Product: Fedora
           Version: rawhide
          Platform: Unspecified
        OS/Version: Unspecified
            Status: NEW
          Severity: unspecified
          Priority: unspecified
         Component: Package Review
        AssignedTo: [e-mail]
        ReportedBy: [e-mail]
         QAContact: [e-mail]
                CC: [e-mail],
                    [e-mail]
    Classification: Fedora
      Story Points: ---

Spec URL:
http://people.redhat.com/clalance/rubygem-compass-960-plugin/rubygem-compass...
SRPM URL:
http://people.redhat.com/clalance/rubygem-compass-960-plugin/rubygem-compass...
Description:
The 960 Grid System is an effort to streamline web development workflow by
providing commonly used dimensions, based on a width of 960 pixels.
http://960.gs/

[Bug 717645] Review Request: rubygem-compass-960-plugin - Compass compatible Sass port of 960.gs

[Red Hat Bugzilla]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.

https://bugzilla.redhat.com/show_bug.cgi?id=717645

Vít Ondruch  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |[e-mail]
             Blocks|                            |705505

[Bug 717645] Review Request: rubygem-compass-960-plugin - Compass compatible Sass port of 960.gs

[Red Hat Bugzilla]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.

https://bugzilla.redhat.com/show_bug.cgi?id=717645

Vít Ondruch  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |ASSIGNED

--- Comment #1 from Vít Ondruch  2011-06-30 11:26:45 EDT ---
Hi Chris,

by coincidence, I did the same package today before I noticed you did that
yesterday :) So you can take a look for comparison [1]. A few things I noticed
in you spec file:

* Wrong license
* site_libdir is useless for gem packages
* you do not provide -doc subpackage, although I am not sure if we should not
  disable the documentation for this gem at all
* %defattr is no more needed except EPEL-4
* BuildRoot is deprecated, except EPEL-5 and older I guess
* Clean section is not required anymore
* There is better to use %{geminstdir} macro in %files section

[1] http://people.redhat.com/vondruch/rubygem-compass-960-plugin.spec

[Bug 717645] Review Request: rubygem-compass-960-plugin - Compass compatible Sass port of 960.gs

[Red Hat Bugzilla]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.

https://bugzilla.redhat.com/show_bug.cgi?id=717645

--- Comment #2 from Vít Ondruch  2011-06-30 11:27:55 EDT ---
And you did not specified ruby(abi)

[Bug 717645] Review Request: rubygem-compass-960-plugin - Compass compatible Sass port of 960.gs

[Red Hat Bugzilla]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.

https://bugzilla.redhat.com/show_bug.cgi?id=717645

--- Comment #3 from Chris Lalancette  2011-06-30 11:56:24 EDT ---
(In reply to comment #1)
> Hi Chris,
>
> by coincidence, I did the same package today before I noticed you did that
> yesterday :) So you can take a look for comparison [1]. A few things I noticed
> in you spec file:
> Wrong license

Oops, fixed.
> site_libdir is useless for gem packages

Yep, removed.
> you do not provide -doc subpackage, although I am not sure if we should not
> disable the documentation for this gem at all

Hm, I'm really fine either way.  I guess it does make sense to have the -doc
package, like other rubygems.  I've taken that bit from yours and put it into
mine.
> %defattr is no more needed except EPEL-4
> BuildRoot is deprecated, except EPEL-5 and older I guess
> Clean section is not required anymore

Heh, I had no idea.  But re-reading the packaging guidelines, I see that this
is indeed the case.  I've removed all 3 of these.
> There is better to use %{geminstdir} macro in %files section

Fixed.

(In reply to comment #2)
> And you did not specified ruby(abi)

Right, fixed.

I've re-uploaded the SPEC and SRPM to the same place; can you take a look when
you get a chance?

Thanks,
Chris Lalancette

[Bug 717645] Review Request: rubygem-compass-960-plugin - Compass compatible Sass port of 960.gs

[Red Hat Bugzilla]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.

https://bugzilla.redhat.com/show_bug.cgi?id=717645

Vít Ondruch  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
               Flag|                            |fedora-review+

--- Comment #4 from Vít Ondruch  2011-06-30 12:14:39 EDT ---
(In reply to comment #3)
> > you do not provide -doc subpackage, although I am not sure if we should not
> > disable the documentation for this gem at all
>
> Hm, I'm really fine either way.  I guess it does make sense to have the -doc
> package, like other rubygems.  I've taken that bit from yours and put it into
> mine.

I just checked the content of the documentation when I did the package myself
and there was virtually nothing, that why I doubt if it would not be better to
disable it at all, but on the other hand, it is not installed by default. So
lets keep it the way it is now.

And I found yet another minor nit. You keep the .gemspec file which is in gem
folder but it has no meaning in our package, so I suggest to remove it:
%exclude %{geminstdir}/%{gemname}.gemspec

And the final one, you are using "Requires: rubygems" where it should be better
to use the virtual provide "Requires: ruby(rubygems)". The same apply for BR.

Otherwise the package looks good. So I APPROVE it.

[Bug 717645] Review Request: rubygem-compass-960-plugin - Compass compatible Sass port of 960.gs

[Red Hat Bugzilla]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.

https://bugzilla.redhat.com/show_bug.cgi?id=717645

--- Comment #5 from Chris Lalancette  2011-06-30 13:23:59 EDT ---
Thanks Vit.  I'll fix those two nits before I import the package.

Chris Lalancette

[Bug 717645] Review Request: rubygem-compass-960-plugin - Compass compatible Sass port of 960.gs

[Red Hat Bugzilla]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.

https://bugzilla.redhat.com/show_bug.cgi?id=717645

Chris Lalancette  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
               Flag|                            |fedora-cvs?

--- Comment #6 from Chris Lalancette  2011-07-01 09:25:50 EDT ---
New Package SCM Request
=======================
Package Name: rubygem-compass-960-plugin
Short Description: Compass compatible Sass port of 960.gs
Owners: clalance vondruch
Branches:
InitialCC:

[Bug 717645] Review Request: rubygem-compass-960-plugin - Compass compatible Sass port of 960.gs

[Red Hat Bugzilla]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.

https://bugzilla.redhat.com/show_bug.cgi?id=717645

--- Comment #7 from Jon Ciesla  2011-07-01 10:11:40 EDT ---
Git done (by process-git-requests).

Note to Vit:  Take ownership of Review bugs you're offically working on
and/or approving.

[Bug 717645] Review Request: rubygem-compass-960-plugin - Compass compatible Sass port of 960.gs

[Red Hat Bugzilla]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.

https://bugzilla.redhat.com/show_bug.cgi?id=717645

--- Comment #8 from Vít Ondruch  2011-07-01 10:19:28 EDT ---
(In reply to comment #7)
> Git done (by process-git-requests).
>
> Note to Vit:  Take ownership of Review bugs you're offically working on
> and/or approving.

Hmm, strange ... assigned doesn't mean assigned to me? :) Will try to do better
next time.

[Bug 717645] Review Request: rubygem-compass-960-plugin - Compass compatible Sass port of 960.gs

[Red Hat Bugzilla]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.

https://bugzilla.redhat.com/show_bug.cgi?id=717645

Vít Ondruch  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
         AssignedTo|[e-mail]    |[e-mail]

[Bug 717645] Review Request: rubygem-compass-960-plugin - Compass compatible Sass port of 960.gs

[Red Hat Bugzilla]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.

https://bugzilla.redhat.com/show_bug.cgi?id=717645

Chris Lalancette  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|ASSIGNED                    |CLOSED
         Resolution|                            |RAWHIDE
        Last Closed|                            |2011-07-13 14:09:15