Note: This is a beta release of Red Hat Bugzilla 5.0. The data contained within is a snapshot of the live data so any changes you make will not be reflected in the production Bugzilla. Also email is disabled so feel free to test any aspect of the site that you want. File any problems you find or give feedback here.
Bug 1188524 - Review Request: nodejs-date-tokens - Convenient date formatting for templates [NEEDINFO]
Summary: Review Request: nodejs-date-tokens - Convenient date formatting for templates
Keywords:
Status: CLOSED NOTABUG
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Piotr Popieluch
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks: FE-DEADREVIEW
TreeView+ depends on / blocked
 
Reported: 2015-02-03 06:40 UTC by anish
Modified: 2016-01-07 10:35 UTC (History)
5 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2016-01-07 10:35:29 UTC
piotr1212: needinfo? (apatil)


Attachments (Terms of Use)

Description anish 2015-02-03 06:40:57 UTC
Spec URL: https://anishpatil.fedorapeople.org/nodejs-date-tokens.spec
SRPM URL: https://anishpatil.fedorapeople.org/nodejs-date-tokens-0.0.2-1.fc21.src.rpm
Description: Date tokens offers convenient ways to use date functions in mustache style templates
Fedora Account System Username:anishpatil

Comment 1 Piotr Popieluch 2015-02-25 21:21:30 UTC
Package fails to build with mock. You can run mock with:
mock --rebuild nodejs-date-tokens-0.0.2-1.fc21.src.rpm
to check if your package will build correctly. Please do a mock build before issue'ing a review request.

The error is caused by a missing 
BuildRequires: nodejs-packaging


rpmlint gives an error, you can run:
rpmlint package.spec package.rpm package.src.rpm
to check if your package is ok.

rpmlint ~/rpmbuild/SPECS/nodejs-date-tokens.spec ~/rpmbuild/SRPMS/nodejs-date-tokens-0.0.2-1.fc21.src.rpm ~/rpmbuild/RPMS/noarch/nodejs-date-tokens-0.0.2-1.fc21.noarch.rpm 
nodejs-date-tokens.src: E: description-line-too-long C Date tokens offers convenient ways to use date functions in mustache style templates
nodejs-date-tokens.noarch: E: description-line-too-long C Date tokens offers convenient ways to use date functions in mustache style templates
nodejs-date-tokens.noarch: W: only-non-binary-in-usr-lib
2 packages and 1 specfiles checked; 2 errors, 1 warnings.

Comment 2 anish 2015-02-26 07:27:21 UTC
Hi Piotr,

Thank you for package review, please find upadated spec and srpms on 

Spec URL: https://anishpatil.fedorapeople.org/nodejs-date-tokens.spec
SRPM URL: https://anishpatil.fedorapeople.org/nodejs-date-tokens-0.0.2-2.fc21.src.rpm

Comment 3 Piotr Popieluch 2015-02-26 21:38:51 UTC
Could you please remove the second %check section.

I have enabled the tests and they fail:

  date-tokens
    + dt
      ✓ should return the tokens object of functions 
      ✓ should return the tokens object of functions, if function called, should return current time 
      ✓ should return the tokens object of functions, if function called without param, should use param in dt() 
      ✓ should return the tokens object of functions, if function called with param, should use param 
      ✓ should return the tokens object of functions with tokens named with the prefix 
      ✓ should return the tokens object of functions with tokens named with the prefix and current time 
    + eval
      ✓ should return the tokens object of strings of eval'd dates 
      ✓ should return the tokens object of strings and should return current time 
      1) should return the tokens object of strings should use param in eval()
      ✓ should return the tokens object of strings with tokens named with the prefix 
      ✓ should return the tokens object of strings with tokens named with the prefix and return current 


  10 passing (14ms)
  1 failing

  1) date-tokens + eval should return the tokens object of strings should use param in eval():
     AssertionError: false == true
      at Context.<anonymous> (/home/piotr/rpmbuild/BUILD/node-date-tokens-0.0.2/test/date-tokens.test.js:86:13)
      at callFn (/usr/lib/node_modules/mocha/lib/runnable.js:223:21)
      at Test.Runnable.run (/usr/lib/node_modules/mocha/lib/runnable.js:216:7)
      at Runner.runTest (/usr/lib/node_modules/mocha/lib/runner.js:374:10)
      at /usr/lib/node_modules/mocha/lib/runner.js:452:12
      at next (/usr/lib/node_modules/mocha/lib/runner.js:299:14)
      at /usr/lib/node_modules/mocha/lib/runner.js:309:7
      at next (/usr/lib/node_modules/mocha/lib/runner.js:247:23)
      at Object._onImmediate (/usr/lib/node_modules/mocha/lib/runner.js:276:5)
      at processImmediate [as _immediateCallback] (timers.js:345:15)




Could you check why they fail? Tests are a good indicator if the library actually works as intended.

Comment 4 anish 2015-02-27 07:12:40 UTC
Thank you for your comments, test case is failing becuase there is a problem with test case result itself, I have created a new pull request in upstream to fix test case itself.
https://github.com/jprichardson/node-date-tokens/pull/2


Please fine new SRPM and spec as 

Spec URL: https://anishpatil.fedorapeople.org/nodejs-date-tokens.spec
SRPM URL: https://anishpatil.fedorapeople.org/nodejs-date-tokens-0.0.2-3.fc21.src.rpm

Comment 5 Piotr Popieluch 2015-02-27 22:33:56 UTC
Good work for fixing the test!

Could you add the missing symlink for the dependencies?

https://fedoraproject.org/wiki/Packaging:Node.js?rd=Node.js/Packagers#Symlinking_Dependencies

Comment 6 anish 2015-03-02 12:23:51 UTC
Hi Piotr,

Thank you!! Please find updated rpms and sprms on 


Spec URL: https://anishpatil.fedorapeople.org/nodejs-date-tokens.spec
SRPM URL: https://anishpatil.fedorapeople.org/nodejs-date-tokens-0.0.2-4.fc21.src.rpm

Comment 7 Piotr Popieluch 2015-03-08 18:13:43 UTC
Please update Source0 to comply with github guidelines:
https://fedoraproject.org/wiki/Packaging:SourceURL?rd=Packaging/SourceURL#Github

Please move LICENSE from %doc to %license:
https://fedoraproject.org/wiki/Packaging:LicensingGuidelines


Please remove test/ from %doc. It's not needed to include tests in %doc

Comment 9 Piotr Popieluch 2015-07-06 19:38:39 UTC
Anish, any updates on this?

Comment 10 Piotr Popieluch 2015-10-17 11:53:17 UTC
This seems stalled, closing bug in one week.
https://fedoraproject.org/wiki/Policy_for_stalled_package_reviews


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