openlp-core team mailing list archive

[Tomas Groth <tomasgroth@xxxxxxxx>]

Review: Needs Fixing

Hi Fabian,

Thanks for contributing to OpenLP! I think this is your first merge request so I'll just start out by saying that all merge requests goes through this review process, and most of them has to be changed and resubmitted one or several times before getting merged. This means that your code (not you!) most likely will meet criticism and you will need to add/rewrite some of it before it can be merged. This can be a long process when adding new features, but I hope you will get through it! :)

Raoul has already given you a lot of stuff to do, so I'll (try to) keep it short...

Regarding code-style, try running "pep8" in the root of openlp to see if your code is compliant with the standards.

I had a look at planningcenteronline and there seems to be 2 APIs a new and an old one. Which one are you using? Does it require a paid subscription to use the API? Can I just create an account and start using it with OpenLP and this plugin?

To make your string translatable wrap them in a "translate()" call, see other plugins for how it works.

And last but not least: Tests. Nothing new gets added to OpenLP without some tests that proves that the code works as expected, so you'll have to write some tests. Take a look at the existing tests to see how it can be done. 
-- 
https://code.launchpad.net/~supagu/openlp/planningcenter-branch/+merge/272094
Your team OpenLP Core is subscribed to branch lp:openlp.