[Bug 1196289] Review Request: nodejs-defaults - A simple one level options merge utility

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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

Lubomir Rintel <lkundrak@xxxxx> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Blocks|177841 (FE-NEEDSPONSOR)     |
              Flags|fedora-review?              |fedora-review+



--- Comment #6 from Lubomir Rintel <lkundrak@xxxxx> ---
Looks better. A couple of minor issues:

1.) Please don't use too long lines in changelogs; they are difficult to use.

Also, you need to escape "%", otherwise it causes a macro expansion.
Everywhere.

Changelog entry like this would probably be better:

* Tue Mar 17 2015 Zuzana Svetlikova <zsvetlik@xxxxxxxxxx> - 1.0.0-3
- Added %%check, %%license, nodejs-clone dependency
- changed ExclusiveArch
- removed Group, BuildRoot and %%defattr
- fixed dependency on nodejs-packaging

2.) Why are the tests conditional?

It sometimes makes sense to disable the tests by default when the dependencies
are not satisfied yet, but not here.

Please drop the %enable_tests macro.

These seem to be fairly minor and uncontroversial issues, fine to fix them
post-import.

APPROVED


Referenced Bugs:

https://bugzilla.redhat.com/show_bug.cgi?id=177841
[Bug 177841] Tracker: Review requests from new Fedora packagers who need a
sponsor
-- 
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





[Index of Archives]     [Fedora Legacy]     [Fedora Desktop]     [Fedora SELinux]     [Yosemite News]     [KDE Users]     [Fedora Tools]