https://bugzilla.redhat.com/show_bug.cgi?id=1267036 Petr Šabata <psabata@xxxxxxxxxx> changed: What |Removed |Added ---------------------------------------------------------------------------- Summary|Review Request: |Review Request: |perl-Mojolicious-Plugin-Ass |perl-Mojolicious-Plugin-Ass |etPack - Compress and |etPack - Compress and |convert css, less, sass, |convert CSS, Less, Sass, |javascript and coffeescript |JavaScript and CoffeeScript |files |files Flags|fedora-review? |fedora-review+ --- Comment #3 from Petr Šabata <psabata@xxxxxxxxxx> --- (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. -- You are receiving this mail because: You are on the CC list for the bug. You are always notified about changes to this product and component _______________________________________________ package-review mailing list package-review@xxxxxxxxxxxxxxxxxxxxxxx https://admin.fedoraproject.org/mailman/listinfo/package-review