Bug 1267036 - Review Request: perl-Mojolicious-Plugin-AssetPack - Compress and convert CSS, Less, Sass, JavaScript and CoffeeScript files
Summary: Review Request: perl-Mojolicious-Plugin-AssetPack - Compress and convert CSS,...
Keywords:
Status: CLOSED RAWHIDE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Petr Šabata
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks: 1267037
TreeView+ depends on / blocked
 
Reported: 2015-09-28 22:00 UTC by Emmanuel Seyman
Modified: 2015-10-26 09:03 UTC (History)
2 users (show)

Fixed In Version:
Clone Of:
Environment:
Last Closed: 2015-10-26 09:03:42 UTC
Type: ---
Embargoed:
psabata: fedora-review+


Attachments (Terms of Use)

Description Emmanuel Seyman 2015-09-28 22:00:11 UTC
Spec URL: http://people.parinux.org/~seyman/fedora/perl-Mojolicious-Plugin-AssetPack/perl-Mojolicious-Plugin-AssetPack.spec
SRPM URL: http://people.parinux.org/~seyman/fedora/perl-Mojolicious-Plugin-AssetPack/perl-Mojolicious-Plugin-AssetPack-0.68-1.fc22.src.rpm
Description:
Mojolicious::Plugin::AssetPack is a Mojolicious plugin which can be used to
cram multiple assets of the same type into one file. This means that if you
have a lot of CSS files (.css, .less, .sass, ...) as input, the AssetPack
can make one big CSS file as output. This is good, since it will often
speed up the rendering of your page. The output file can even be minified,
meaning you can save bandwidth and browser parsing time.

Fedora Account System Username: eseyman

Comment 1 Petr Šabata 2015-09-29 11:40:18 UTC
* Correct the lettercase in your description.
 `CSS', `Less', `Sass', `JavaScript' and `CoffeeScript'.

* Missing a builddep called in the spec file: make

* Since you're using the NO_PACKLIST feature, require at least EU::MM 6.76.

* Missing builddeps needed for the %check phase:
 - perl(Fcntl), lib/Mojolicious/Plugin/AssetPack/Asset.pm:5
 - perl(IO::File), lib/Mojolicious/Plugin/AssetPack/Asset.pm:6
 - perl(Mojo::EventEmitter), lib/Mojolicious/Plugin/AssetPack/Preprocessors.pm:70
 - perl(Mojo::UserAgent), lib/Mojolicious/Plugin/AssetPack.pm:26
 - perl(Mojolicious::Plugin), lib/Mojolicious/Plugin/AssetPack.pm:3
 - perl(POSIX), lib/Mojolicious/Plugin/AssetPack/Preprocessor.pm:19

* Carp is not used anywhere.  You may (and should) drop it.

* Missing an undetected runtime dependency:
 - perl(Mojo::UserAgent), lib/Mojolicious/Plugin/AssetPack.pm:26

* I see many tests are skipped due to missing preprocessors.  Makes me
  wonder whether this module actually works for these.
  You may want to require some or all of them (if available), both at
  build and runtime.

  t/coffee.t ..................... skipped: Could not find preprocessors for coffee
  t/jsx.t ........................ skipped: Could not find preprocessors for jsx
  t/less.t ....................... skipped: Could not find preprocessors for less
  t/sass.t ....................... skipped: Could not find preprocessors for sass
  t/scss-no-cache.t .............. skipped: Could not find preprocessors for scss
  t/scss.t ....................... skipped: Could not find preprocessors for scss

  For CoffeeScript it would be the `coffee-script' package.
  For Jsx... we don't have anything in Fedora, it seems.
  For Less it would be the `nodejs-less' package.
  For Sass and Scss it would be the Css::Sass Perl module, not yet packaged.

* Consider running the optional POD tests (see t/00-basic.t).

* Consider packaging the examples directory as %doc.

Comment 2 Emmanuel Seyman 2015-09-29 16:48:10 UTC
(In reply to Petr Šabata from comment #1)
>
> * Correct the lettercase in your description.
>  `CSS', `Less', `Sass', `JavaScript' and `CoffeeScript'.

Done.

> * Missing a builddep called in the spec file: make

Added.

> * Since you're using the NO_PACKLIST feature, require at least EU::MM 6.76.

Indeed. Added.


> * Missing builddeps needed for the %check phase:
>  - perl(Fcntl), lib/Mojolicious/Plugin/AssetPack/Asset.pm:5
>  - perl(IO::File), lib/Mojolicious/Plugin/AssetPack/Asset.pm:6
>  - perl(Mojo::EventEmitter),
> lib/Mojolicious/Plugin/AssetPack/Preprocessors.pm:70
>  - perl(Mojo::UserAgent), lib/Mojolicious/Plugin/AssetPack.pm:26
>  - perl(Mojolicious::Plugin), lib/Mojolicious/Plugin/AssetPack.pm:3
>  - perl(POSIX), lib/Mojolicious/Plugin/AssetPack/Preprocessor.pm:19

I've added all of these.

> * Carp is not used anywhere.  You may (and should) drop it.

Removed.

> * Missing an undetected runtime dependency:
>  - perl(Mojo::UserAgent), lib/Mojolicious/Plugin/AssetPack.pm:26

Added.

> * I see many tests are skipped due to missing preprocessors.  Makes me
>   wonder whether this module actually works for these.
>   You may want to require some or all of them (if available), both at
>   build and runtime.

I was unwilling to add these as runtime requirements because users would end up having to install all of these even if they just want to pack one type of files. Maybe this is a case for soft requirements.

Adding them as BuildRequires has no impact on users so I've added those.

> * Consider running the optional POD tests (see t/00-basic.t).

Done.

> * Consider packaging the examples directory as %doc.

Done.

Spec URL: http://people.parinux.org/~seyman/fedora/perl-Mojolicious-Plugin-AssetPack/perl-Mojolicious-Plugin-AssetPack.spec
SRPM URL: http://people.parinux.org/~seyman/fedora/perl-Mojolicious-Plugin-AssetPack/perl-Mojolicious-Plugin-AssetPack-0.68-2.fc22.src.rpm

Comment 3 Petr Šabata 2015-09-30 11:53:10 UTC
(In reply to Emmanuel Seyman from comment #2)
> (In reply to Petr Šabata from comment #1)
> >
> > * Correct the lettercase in your description.
> >  `CSS', `Less', `Sass', `JavaScript' and `CoffeeScript'.
> 
> Done.

Ack.  Also changing the summary of this bug.

> > * Missing a builddep called in the spec file: make
> 
> Added.

Ack.
 
> > * Since you're using the NO_PACKLIST feature, require at least EU::MM 6.76.
> 
> Indeed. Added.

Ack.

> > * Missing builddeps needed for the %check phase:
> >  - perl(Fcntl), lib/Mojolicious/Plugin/AssetPack/Asset.pm:5
> >  - perl(IO::File), lib/Mojolicious/Plugin/AssetPack/Asset.pm:6
> >  - perl(Mojo::EventEmitter),
> > lib/Mojolicious/Plugin/AssetPack/Preprocessors.pm:70
> >  - perl(Mojo::UserAgent), lib/Mojolicious/Plugin/AssetPack.pm:26
> >  - perl(Mojolicious::Plugin), lib/Mojolicious/Plugin/AssetPack.pm:3
> >  - perl(POSIX), lib/Mojolicious/Plugin/AssetPack/Preprocessor.pm:19
> 
> I've added all of these.

Ack.

> > * Carp is not used anywhere.  You may (and should) drop it.
> 
> Removed.

Ack.

> > * Missing an undetected runtime dependency:
> >  - perl(Mojo::UserAgent), lib/Mojolicious/Plugin/AssetPack.pm:26
> 
> Added.

Ack.

> > * I see many tests are skipped due to missing preprocessors.  Makes me
> >   wonder whether this module actually works for these.
> >   You may want to require some or all of them (if available), both at
> >   build and runtime.
> 
> I was unwilling to add these as runtime requirements because users would end
> up having to install all of these even if they just want to pack one type of
> files. Maybe this is a case for soft requirements.
> 
> Adding them as BuildRequires has no impact on users so I've added those.

Indeed, weak dependencies would be the way to go.  Now just choose which variant :)

> > * Consider running the optional POD tests (see t/00-basic.t).
> 
> Done.

Ack.

> > * Consider packaging the examples directory as %doc.
> 
> Done.

Ack.

--

No blockers, approving.
I think I'll package CSS::Sass so you can add a BR/wR later.

Comment 4 Petr Šabata 2015-09-30 11:57:08 UTC
(In reply to Petr Šabata from comment #3)
> I think I'll package CSS::Sass so you can add a BR/wR later.

I've just looked at it and... I think I'll pass ;)

Comment 5 Emmanuel Seyman 2015-09-30 12:19:25 UTC
(In reply to Petr Šabata from comment #3)
>
> No blockers, approving.

Thanks! I've requested a new package in pkgdb.

Comment 6 Emmanuel Seyman 2015-10-26 09:03:42 UTC
Package imported and built in Rawhide.


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