openlp-core team mailing list archive
-
openlp-core team
-
Mailing list archive
-
Message #02897
Re: [Merge] lp:~phill-ridout/openlp/wow_import into lp:openlp
Review: Needs Fixing
- Your class' docstring needs to be reStructuredText.
- Please make your file variable more descriptive, "f" is not enough.
- If BLOCK_TYPES is supposed to be a constant, put it at module level, where it will be instantiated once,
rather than at function level where it will be instantiates every time the function is run.
- AFAIK, "with" is not supported on Python 2.5 (other than through the __future__ module). Please use the
standard "file = open(...)".
- The do_import_file function is indented 8 spaces. The standard is 4.
- As I mentioned in IRC, the do_import function needs to take a list of file names, not a single file name.
- WowImport must inherit from SongImport.
- "z" is not a valid variable name, please be more descriptive.
- Does the zip file object not need to be closed?
--
https://code.launchpad.net/~phill-ridout/openlp/wow_import/+merge/32051
Your team OpenLP Core is subscribed to branch lp:openlp.
References