[Pkg-javascript-devel] Please review libjs-jquery-coolfieldset

[François-Régis]

Hi Emilien,

Sorry for the delay,

Le 11/03/2014 07:40, Emilien Klein a écrit :
> Hi François-Régis,
> 
> 2014-03-07 17:33 GMT+01:00 François-Régis <frv-debian at miradou.com>:
>> Could anybody have a look at libjs-jquery-coolfieldset [1], it's my
>> first js package so please look it closely...
> 
> I've looked at your package (not tried to build it), here are a couple
> of comments:

Thank you for the time you've spend on my package.

> * d/copyright:
>   - the copyright of each file in the upstream tarball (even if it's
> not present in the Debian binary package) must be mentioned. jQuery
> (file js/jquery.js) must be mentioned.

I've changed it but feel it strange to have a
/usr/share/doc/[pagkage-name]/copyright mentioning files that doesn't
exist. (and I don't find find the debian policy on it)

>   - You are not specifying the copyright of the files in the Debian
> package. I expect those to be under your copyright, possibly using the
> same license as upstream?

Yes, I follow an advice of a DD on another of my packages meaning I
don't care claiming copyright on packaging stuff.

>   - Upstream-Name: You've specified the same name as the Debian
> package, but I don't see that name back on
> http://w3shaman.com/article/jquery-plugin-collapsible-fieldset .
> Looking at the name of the upstream tarball and the line "By the way,
> I named this plugin coolfieldset." I'd guess it should be
> "coolfieldset".

You're right I've changed

>   - Question about Upstream-Contact: Should it be Luc Ky, or (as
> specified in the copyright mention in file js/jquery.coolfieldset.js:
> "Lucky"?

>From what I've seen he is Luc Ky but this is part of the problem I
detail below. By the way I don't know if we should prefer the nickname
or the real firstname lastname identity.

> * d/control:
>   - About the name of the package: Looking at the packages maintained
> in our team's Git repo [0], most source packages have a name that
> doesn't include "libjs-", while the binary package does. I'm wondering
> if your source package should not just be named "jquery-coolfieldset",
> while the source package remains "libjs-jquery-coolfieldset"?

Ok I did'nt catch it. Changed source package name.

>   - Homepage: You link to the .js file on Google Code. Wouldn't this
> be a better homepage, as it contains examples and all:
> http://w3shaman.com/article/jquery-plugin-collapsible-fieldset

OK done, in fact searching for this plugin wich is a depend of xxx I've
first found the Google Code page which is not the genuine source.

>   - The Vcs-Browser link returns a 404 error (hint: not in collab-maint)

Done

>   - Same issue for your Vcs-Git link

Also done

>   - 2 typo in the Description: "This plugin can collapsed or hide
> fieldset" -> "This plugin can collapse or hide fieldsets"
>   - 3 other typo: "The separated CSS file will also useful if you need
> to modify the fieldset appearance." -> "The separate CSS file will
> also be useful if you need to modify the fieldset's appearance."

Ok done

>   - In my js package [1], I've added "Suggests: javascript-common".
> Can't remember if that was a suggestion, or if I saw that in some
> other Debian js package. Please investigate if this would provide any
> benefit to the user.	

libjs-jquery has a recommend dependcy on javascript-common so I think it
doesn'nt worth making another dependency on package depending of jwquery
but I may be wrong cause 124 packages have recommend dependency on
javascript-common in wich lots of jquery plugins.

By the way there are only two packages with suggest dependency on j-c,
yours andlibjs-raphael.

> 
> * d/rules:
>   - you use uglifyjs, but it's not indicated in d/control's
> Build-Depends (node-uglify)

Well seen, done

>   - I find it a bit suprising that the build directory is in the root,
> not the debian directory. Is that on purpose? (my packages build
> inside a debian/tmp-build folder, so as to not change the upstream
> structure more than adding the debian folder). I don't know if there
> is a strong Debian policy on that, input from others on the team
> welcome.

I've did that way from other packages but I think you're right, even if
there is no strong policy, it's easier to build inside the debian
directory. I've changed it.


>   - Why are you deleting js/*.min.js in override_dh_auto_clean?
> Looking at your call to uglifyjs, it looks like only one minified file
> will be created, in the build folder (not js folder)?

Just a garbage from my previous try to build minify version in js/ .
Removed it.

> * d/README.source: read the content of the file, and take appropriate action

OK done

> * d/docs: empty file?

Deleted

> * d/install: see comment about build folder in d/rules
> 
> * d/watch:
>   - A Github repository is mentioned, that seems to be a fork. But
> that is not mentioned anywhere (namely in d/copyright, d/control and
> d/README.source)

You're tight I shoujd have explained it but I don't knox where : You
perhaps have noticed that Lucky provide a single tarball wich is not the
pristine one but amended with part of the comments on his article but
not all of them. I've mailed him to know if he has a scs repository or
anithing like that but have no response at time. In between I've build a
github repo yp br sure to keep the important fixes mentionned in
comments. That is not a real fork but just a secure option.


>   - Also, if you plan to look at Github from your watchfile, please
> use the Debian githubredir service.

Thank you for the tip, I did'nt know this service. Is it mandatory to
use it or just a service when no tarball is available ftom github ?


> Hope this helps. Let us know on the list if you have questions, or
> don't agree with my comments.

Thankx a lot for your help and comments.

-- 
Framçois-Régis