Review request: Kcmgrub2
Alberto Mattea
alberto at mattea.info
Wed Apr 6 14:03:03 BST 2011
Hi, thanks for the review.
In data mercoledì 6 aprile 2011 00:28:21, Raphael Kubo da Costa ha scritto:
> Alberto Mattea <alberto at mattea.info> writes:
> > Hi all,
> > after 4 releases I think kcmgrub2 has reached an acceptable level of
> > maturity, so I'd ask for a move to kdereview. It is currently in
> > playground-sysadmin (git).
>
> I only took a quick look, as my Py{Qt,KDE}-fu is not that good.
>
> Buildsystem-wise:
>
> * I did not understand why you used include() instead of
> find_package() in, for example,
>
> include(FindPyQt4)
Actually I used an article on the KDE wiki as an example, I didn't know it
wasn't the standard way of doing it. Now it's fixed.
>
> * It's probably a good idea to add some kind of README for packagers
> explaining what the dependencies are.
Ok, done.
>
> Licensing-wise:
>
> * Don't you need to add the appropriate license header to your code
> files?
Yep :-). Done.
>
> As for kcmgrub2.py itself:
>
> * It might be better to set the WhatsThis values in the .ui file
> itself.
Well, I thought about this, but ultimately I chose to put them in the code so
that in the future I may make them dynamic (example: if python-xlib is not
available explain why in the resolution whatsthis).
> * `x' is not a good name for a global variable.
You're right, I changed it to xlibAvailable.
Thanks again,
Alberto
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
URL: <http://mail.kde.org/pipermail/kde-core-devel/attachments/20110406/7455a527/attachment.sig>
More information about the kde-core-devel
mailing list