Code review comment for lp:~joni-noplu/moovida/moovida-dvb

Revision history for this message
Jonathan Rauprich (joni-noplu) wrote :

Another short question: Should i delete this proposal?

On Sun, 2010-05-30 at 17:13 +0000, Olivier Tilloy wrote:
> Hi Jonathan,
> Support for DVB looks like an amazing feature (I recall it being discussed extensively but resources were never allocated for implementation).
> I haven't tested your code, but I have a bunch of important remarks:
>
> 1) In case you didn't know, Moovida 1.x is not supported any longer by Fluendo Embedded (see http://www.moovida.com/blog/2010/03/26/moovida-update/ and an interesting thread on the developers' mailing list: http://lists.moovida.com/pipermail/developers-list/2010-March/001804.html). Basically that means that your best bet to get your work reviewed is to count on the community. However a large part of said community is now focused on the fork (temporarily named Elisa until a better name is agreed upon: https://launchpad.net/elisa), and the ongoing efforts are focused on:
> a) Fixing the outstanding issues in the 1.0 branch to keep it usable (no new features)
> b) Brainstorming and designing for the next version
>
> 2) Your patch mixes new features (the new gdvbd plugin) and unrelated fixes, I'd advise to submit each as separate merge requests, it will ease the life of potential reviewers.
>
> 3) For the reasons detailed in 1, you probably want to submit bug fixes against lp:elisa, you'll stand a better chance to have them reviewed and merged.
>
> 4) The fix for the amp plugin is already merged in lp:elisa, see http://bazaar.launchpad.net/~elisa-developers/elisa/1.0/revision/1616
>
> 5) A fix for bug #379409 (CSS values with no units) is already pending a review, see https://code.launchpad.net/~osomon/elisa/css_property_no_unit/+merge/26369
>
> 6) There is a bug report for the issue with teletext (bug #415031). It is in general a good practice to link to existing bug reports in the merge request. Again, this will make the life of the reviewer(s) much easier.
>
> 7) Your branch is missing the setup.py script for the new gdvbd plugin. If you haven't done so yet, I encourage you to go through the good practices detailed in the developers' guide at http://www.moovida.com/documentation/developers_guide/developers_guide.html
>
> 8) The plugin shouldn't be merged in elisa/moovida's tree: it is preferable to create a standalone project in Launchpad to host its code, track its issues separately and release it in an independent manner. This project should be declared as part of the Moovida Universe meta-project (https://launchpad.net/moovida-universe).
>
> 9) The change to the LIRC plugin seems unrelated, and in any case should be carefully explained, because it touches a sensitive piece of code.
>
> Thanks for reading patiently my remarks! Some of them might sound picky, but please know that your effort is valued, as a former core developer on the project it is very rewarding to see such cool contributions coming in. If you're interested in the fork, drop by on irc://freenode.net/#elisa, there's always someone hanging around there.

« Back to merge proposal