Bug 1267036

Summary:	Review Request: perl-Mojolicious-Plugin-AssetPack - Compress and convert CSS, Less, Sass, JavaScript and CoffeeScript files
Product:	[Fedora] Fedora	Reporter:	Emmanuel Seyman <emmanuel>
Component:	Package Review	Assignee:	Petr Šabata <psabata>
Status:	CLOSED RAWHIDE	QA Contact:	Fedora Extras Quality Assurance <extras-qa>
Severity:	medium	Docs Contact:
Priority:	medium
Version:	rawhide	CC:	package-review, psabata
Target Milestone:	---	Flags:	psabata: fedora-review+
Target Release:	---
Hardware:	All
OS:	Linux
Whiteboard:
Fixed In Version:		Doc Type:	Bug Fix
Doc Text:		Story Points:	---
Clone Of:		Environment:
Last Closed:	2015-10-26 09:03:42 UTC	Type:	---
Regression:	---	Mount Type:	---
Documentation:	---	CRM:
Verified Versions:		Category:	---
oVirt Team:	---	RHEL 7.3 requirements from Atomic Host:
Cloudforms Team:	---	Target Upstream Version:
Embargoed:
Bug Depends On:
Bug Blocks:	1267037

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.