[Pkg-javascript-devel] Bug#890539: RFS: node-yamljs/0.3.0+dfsg-1 [ITP]

[Bastien ROUCARIES]

first mail incomplete complete

On Mon, May 14, 2018 at 9:58 PM, Bastien ROUCARIES
<roucaries.bastien at gmail.com> wrote:
> On Mon, May 14, 2018 at 8:32 PM, Andreas Moog <andreas.moog at warperbbs.de> wrote:
>> On Sun, May 13, 2018 at 09:20:40PM +0200, Bastien ROUCARIES wrote:
>>
>> Thanks for the review. I have a couple of questions about some of the points:
>>
>>> 1. you need a smoke test : aka run a json file and convert to yaml.
>>> create a debian/smoketest dir and put file here
>>
>> 1. Ok, I added 2 more tests to debian/tests, one for yaml2json, one for
>> json2yaml.
>
Not good from security point of view, you need to use mktemp.

And diff does not fail if content change. Better to use cmp tool
>
>>> 2. why did you not run testsuite ?
>>
>> As far as I can tell the testsuite requires node-jasmine, which isn't packaged
>> for Debian.
>
Ok please create a itp or rfp for nde-jasmine and mention this
package. Will sponsor node-jasmine
>
> in all the case add to makefile (this time no vulnerability because
> you write to local build):
> override_dh_auto_test:
>
%.yaml: %.json
    bin/json2yaml $< > $@

%.json: %.yaml
    bin/json2yaml $< > $@

override_dh_auto_clean:
    rm -f debian/testdata/yaml2json.json
    rm -f debian/testdata/json2yaml.yaml
    dh_auto_clean

ifeq (,$(filter nocheck,$(DEB_BUILD_OPTIONS) $(DEB_BUILD_PROFILES)))
override_dh_auto_test: debian/testdata/yaml2json.json
debian/testdata/json2yaml.yaml
     cmp debian/testdata/yaml2json.json debian/testdata/json2yaml.yaml
 else
 override_dh_auto_test:
     @echo '**********************************************************'
     @echo 'Skip test suite                                           '
     @echo '**********************************************************'
 endif

Or something like this
>
>
>>
>>> 3 copyright is not good run on github tree
>>> git log gihtub/master  --format="%an <%ae>" |  sort -u
>>
>> That gives me a list of about 20 contributors, some with only very minor
>> contributions like a typo fix. Where would I make the cutoff point for
>> including them in debian/copyright? Adding all of the contributors feels
>> wrong. The LICENSE only mentions jeremyfa as copyright holder.
>
Better safe then sorry. Please do it
>
>>> 4. copyright year are 2010-2017
>>
>> Fixed.
>>
>>> 5 install is not right you need only to install index.js and lib/ cli/
>>> package.json files under nodes dir
>>
>> Right, I also added the bin dir since the 2 standalone scripts use only
>> relative paths to find the libraries (via "var YAML=require('../lib/Yaml.js')")
>> and I didn't want to diverge from upstream here.

okj

>>> 6. you should also exclude lib if you rebuilt it under copyright file-excluded:
>>
>> Good point, done.
>>
>>> 7. demo.html should be ship with dh_examples
>>
>> It already is in debian/examples, do you mean I should put it somewhere else?

ok

8. demo.html include yui.css in minified form.(see
https://paulund.co.uk/collection-of-css-resets)

Copyright does not mention it. And you fail to build from source.

Sor remove demo.html  using File-Excluded and recreate it from source
removing the css reset code.

you could use the css-reset I am packaged on node-modernize.css and
suggest node-modernize.css

Bastien

>> I pushed an updated repository with the current state of my work.
>>
>> Viele Grüße // Kind Regards
>>
>> Andreas
>>
>> --
>> PGP-encrypted mails preferred
>> PGP Fingerprint: 74CD D9FE 5BCB FE0D 13EE 8EEA 61F3 4426 74DE 6624