openlp-core team mailing list archive
-
openlp-core team
-
Mailing list archive
-
Message #34319
Re: [Merge] lp:~john+ubuntu-g/openlp/singingthefaith into lp:openlp
Review: Needs Fixing
Please change your string formatting to use the 'new' style with the format function. ( https://pyformat.info/ ) also string formatting is preferred over concatenation (i.e, "part1" + var + "part2")
single quotes for strings, not double quotes
do_import_file is very long can this be split in to smaller methods?
Diff comments:
>
> === added file 'openlp/plugins/songs/lib/importers/singingthefaith.py'
> --- openlp/plugins/songs/lib/importers/singingthefaith.py 1970-01-01 00:00:00 +0000
> +++ openlp/plugins/songs/lib/importers/singingthefaith.py 2019-06-30 19:19:46 +0000
> @@ -0,0 +1,347 @@
> +# -*- coding: utf-8 -*-
> +# vim: autoindent shiftwidth=4 expandtab textwidth=120 tabstop=4 softtabstop=4
> +
> +###############################################################################
> +# OpenLP - Open Source Lyrics Projection #
> +# --------------------------------------------------------------------------- #
> +# Copyright (c) 2008-2019 OpenLP Developers #
> +# --------------------------------------------------------------------------- #
> +# This program is free software; you can redistribute it and/or modify it #
> +# under the terms of the GNU General Public License as published by the Free #
> +# Software Foundation; version 2 of the License. #
> +# #
> +# This program is distributed in the hope that it will be useful, but WITHOUT #
> +# ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or #
> +# FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for #
> +# more details. #
> +# #
> +# You should have received a copy of the GNU General Public License along #
> +# with this program; if not, write to the Free Software Foundation, Inc., 59 #
> +# Temple Place, Suite 330, Boston, MA 02111-1307 USA #
> +###############################################################################
> +"""
> +The :mod:`singingthefaith` module provides the functionality for importing songs which are
> +exported from Singing The Faith - an Authorised songbook for the Methodist Church of
> +Great Britain."""
> +
> +import logging
> +import re
> +from pathlib import Path
> +
> +from openlp.core.common.i18n import translate
> +from openlp.plugins.songs.lib.importers.songimport import SongImport
> +
> +log = logging.getLogger(__name__)
> +
> +
> +class SingingTheFaithImport(SongImport):
> + """
> + Import songs exported from SingingTheFaith
> + """
> +
> + hints_available = False
> + checks_needed = True
> + hintline = {}
can we have some consistency here and make these hint_line
> + hintfile_version = '0'
and hint_file_version
> + hint_verseOrder = ''
also please stick to 'snake_case' variable names like hint_verse_order
> + hint_songtitle = ''
hint_song_title
> + hint_comments = ''
> + hint_ignoreIndent = False
hint_ignore_indent
> +
> + def do_import(self):
> + """
> + Receive a single file or a list of files to import.
> + """
> + if not isinstance(self.import_source, list):
> + return
> + self.import_wizard.progress_bar.setMaximum(len(self.import_source))
> + for file_path in self.import_source:
> + if self.stop_import_flag:
> + return
> + with file_path.open('rt', encoding='cp1251') as song_file:
> + self.do_import_file(song_file)
> +
> + def do_import_file(self, file):
> + """
> + Process the SingingTheFaith file - pass in a file-like object, not a file path.
> + """
> + singingTheFaithVersion = 1
singing_the_faith_version
> + self.set_defaults()
> + # Setup variables
> + line_number = 0
> + old_indent = 0
> + # The chorus indent is how many spaces the chorus is indented - it might be 6,
> + # but we test for >= and I do not know how consistent to formatting of the
> + # exported songs is.
> + chorus_indent = 5
> + song_title = 'STF000 -'
> + song_number = '0'
> + ccli = '0'
> + current_verse = ''
> + current_verse_type = 'v'
> + current_verse_number = 1
> + # Potentially we could try to track current chorus number to automatically handle
> + # more than 1 chorus, currently unused.
> + # current_chorus_number = 1
> + has_chorus = False
> + chorus_written = False
> + auto_verse_order_ok = False
> + copyright = ''
> + # the check_flag is prepended to the title, removed if the import should be OK
> + # all the songs which need manual editing should sort below all the OK songs
> + check_flag = 'z'
> +
> + self.add_comment("Imported with Singing The Faith Importer v " + str(singingTheFaithVersion))
> +
> + # Get the file_song_number - so we can use it for hints
> + filename = Path(file.name)
> + song_number_file = filename.stem
> + song_number_match = re.search(r'\d+', song_number_file)
> + if song_number_match:
> + song_number_file = song_number_match.group()
> +
> + # See if there is a hints file in the same location as the file
> + dir_path = filename.parent
> + hints_file_path = dir_path / 'hints.tag'
> + try:
> + with hints_file_path.open('r') as hints_file:
> + hints_available = self.read_hints(hints_file, song_number_file)
> + except FileNotFoundError:
> + hints_available = False
> +
> + try:
> + for line in file:
> + line_number += 1
> + if hints_available and (str(line_number) in self.hintline):
brackets not required on the second part https://docs.python.org/3/reference/expressions.html#operator-precedence
> + hint = self.hintline[str(line_number)]
> + if hint == "Comment":
> + line.strip()
> + self.add_comment(line)
> + line_number += 1
> + next(file)
> + continue
> + elif hint == "Ignore":
> + line_number += 1
> + next(file)
> + continue
> + elif hint == "Author":
> + # add as a raw author - do not split and make them a words author
> + line.strip()
> + self.add_author(line, 'words')
> + line_number += 1
> + next(file)
> + continue
> + elif hint.startswith("VariantVerse"):
> + (vv, hintverse, replace) = hint.split(" ", 2)
> + this_verse = self.verses[int(hintverse) - 1]
> + this_verse_str = this_verse[1]
> + new_verse = this_verse_str
> + # There might be multiple replace pairs separated by |
> + replaces = replace.split("|")
> + for rep in replaces:
> + (source_str, dest_str) = rep.split("/")
brackets not required for tuples here
> + new_verse = new_verse.replace(source_str, dest_str)
> + self.add_verse(new_verse, 'v')
> + self.verse_order_list.append('v' + str(current_verse_number))
> + current_verse_number += 1
> + line_number += 1
> + next(file)
> + continue
> + else:
> + self.log_error(translate('SongsPlugin.SingingTheFaithImport', 'File %s' % file.name),
again, new format here please.
> + translate('SongsPlugin.SingingTheFaithImport', 'Unknown hint %s' % hint))
> + return
> + # STF exported lines have a leading verse number at the start of each verse.
> + # remove them - note that we want to track the indent as that shows a chorus
> + # so will deal with that before stipping all leading spaces.
> + indent = 0
> + if line.strip():
> + verse_num_match = re.search(r'^\d+', line)
> + if verse_num_match:
> + # Could extract the verse number and check it against the calculated
> + # verse number - TODO
> + # verse_num = verse_num_match.group()
> + line = line.lstrip("0123456789")
> + indent_match = re.search(r'^\s+', line)
> + if indent_match:
> + indent = len(indent_match.group())
> + # Assuming we have sorted out what is verse and what is chorus, strip lines,
> + # unless ignoreIndent
> + if self.hint_ignoreIndent:
> + line = line.rstrip()
> + else:
> + line = line.strip()
> + if line_number == 2:
> + # note that songs seem to start with a blank line
> + song_title = line
> + # Detect the 'Reproduced from Singing the Faith Electronic Words Edition' line
> + if line.startswith('Reproduced from Singing the Faith Electronic Words Edition'):
> + song_number_match = re.search(r'\d+', line)
> + if song_number_match:
> + song_number = song_number_match.group()
> + continue
> + # If the indent is 0 and it contains '(c)' then it is a Copyright line
> + elif (indent == 0) and ("(c)" in line):
these, and the following brackets are not required (see the order of precedence I linked to earlier) also this could be simplified to:
elif indent == 0:
if '(c)' in line:
....
elif line.startswith('Liturgical ') or line.startswith('From The ') or line.startswith('From Common '):
....
elif line:
....
> + copyright = line
> + continue
> + elif (indent == 0) and (line.startswith('Liturgical ')):
> + self.add_comment(line)
> + continue
> + elif (indent == 0) and (line.startswith('From The ')):
> + self.add_comment(line)
> + continue
> + elif (indent == 0) and (line.startswith('From Common ')):
> + self.add_comment(line)
> + continue
> + # If indent is 0 it may be the author, unless it was one of the cases covered above
> + elif (indent == 0) and len(line) > 0:
> + # May have more than one author, separated by ' and '
> + authors = line.split(' and ')
> + for a in authors:
> + self.parse_author(a)
> + continue
> + if line == '':
> + if current_verse != '':
> + self.add_verse(current_verse, current_verse_type)
> + self.verse_order_list.append(current_verse_type + str(current_verse_number))
> + if current_verse_type == 'c':
> + chorus_written = True
> + else:
> + current_verse_number += 1
> + current_verse = ''
> + if chorus_written:
> + current_verse_type = 'v'
> + else:
> + # If the line is indented more than or equal chorus_indent then assume it is a chorus
> + # If the indent has just changed then start a new verse just like hitting a blank line
> + if not self.hint_ignoreIndent and ((indent >= chorus_indent) and (old_indent < indent)):
> + if current_verse != '':
> + self.add_verse(current_verse, current_verse_type)
> + self.verse_order_list.append(current_verse_type + str(current_verse_number))
> + if current_verse_type == 'v':
> + current_verse_number += 1
> + current_verse = line
> + current_verse_type = 'c'
> + old_indent = indent
> + chorus_written = False
> + has_chorus = True
> + continue
> + if current_verse == '':
> + current_verse += line
> + else:
> + current_verse += '\n' + line
> + old_indent = indent
> + except Exception as e:
> + self.log_error(translate('SongsPlugin.SingingTheFaithImport', 'File %s' % file.name),
'new' style formatting here please
> + translate('SongsPlugin.SingingTheFaithImport', 'Error: %s') % e)
> + return
> +
> + if self.hint_songtitle:
> + song_title = self.hint_songtitle
> + self.title = check_flag + "STF" + song_number.zfill(3) + " - " + song_title
> + self.song_book_name = "Singing The Faith"
> + self.song_number = song_number
> + self.ccli_number = ccli
> + self.add_copyright(copyright)
> +# If we have a chorus then the generated Verse order can not be used directly, but we can generate
> +# one for two special cases - Verse followed by one chorus (to be repeated after every verse)
> +# of Chorus, followed by verses. If hints for ManualCheck or VerseOrder are supplied ignore this
> + if has_chorus and not self.hint_verseOrder and not self.checks_needed:
> + auto_verse_order_ok = False
> + # Popular case V1 C2 V2 ...
> + if len(self.verse_order_list) >= 1: # protect against odd cases
empty lists evaluate as false so you can just do:
if self.verse_order_list:
....
> + if (self.verse_order_list[0] == "v1") and (self.verse_order_list[1] == "c2"):
> + new_verse_order_list = ['v1', 'c1']
> + i = 2
> + auto_verse_order_ok = True
> + elif (self.verse_order_list[0] == "c1") and (self.verse_order_list[1] == "v1"):
> + new_verse_order_list = ['c1', 'v1', 'c1']
> + i = 2
> + auto_verse_order_ok = True
> + # if we are in a case we can deal with
> + if auto_verse_order_ok:
> + while i < len(self.verse_order_list):
> + if self.verse_order_list[i].startswith('v'):
> + new_verse_order_list.append(self.verse_order_list[i])
> + new_verse_order_list.append("c1")
> + else:
> + # Would like to notify, but want a warning, which we will do via the
> + # Check_needed mechanism, as log_error aborts input of that song.
> + # self.log_error(translate('SongsPlugin.SingingTheFaithImport', 'File %s' % file.name),
> + # 'Error: Strange verse order entry ' + self.verse_order_list[i])
> + auto_verse_order_ok = False
> + i += 1
> + self.verse_order_list = new_verse_order_list
> + else:
> + if not auto_verse_order_ok:
> + self.verse_order_list = []
> + if self.hint_verseOrder:
> + self.verse_order_list = self.hint_verseOrder.split(',')
> + if self.hint_comments:
> + self.add_comment(self.hint_comments)
> + # Write the title last as by now we will know if we need checks
> + if hints_available and not self.checks_needed:
> + check_flag = ''
> + elif not hints_available and not has_chorus:
> + check_flag = ''
> + elif not hints_available and has_chorus and auto_verse_order_ok:
> + check_flag = ''
> + self.title = check_flag + "STF" + song_number.zfill(3) + " - " + song_title
> + if not self.finish():
> + self.log_error(file.name)
> +
> + def read_hints(self, file, song_number):
> + hintfound = False
> + self.hint_verseOrder = ''
> + self.hintline.clear()
> + self.hint_comments = ''
> + self.hint_songtitle = ''
> + self.hint_ignoreIndent = False
> + for tl in file:
> + if not tl.strip():
> + return hintfound
> + tagval = tl.split(':')
> + tag = tagval[0].strip()
> + val = tagval[1].strip()
> + if tag == "Version":
> + self.hintfile_version = val
> + continue
> + if (tag == "Hymn") and (val == song_number):
> + self.add_comment("Using hints version " + str(self.hintfile_version))
> + hintfound = True
> + # Assume, unless the hints has ManualCheck that if hinted all will be OK
> + self.checks_needed = False
> + for tl in file:
> + tagval = tl.split(':')
> + tag = tagval[0].strip()
> + val = tagval[1].strip()
> + if tag == "End":
> + return hintfound
> + elif tag == "CommentsLine":
> + vals = val.split(',')
> + for v in vals:
> + self.hintline[v] = "Comment"
> + elif tag == "IgnoreLine":
> + vals = val.split(',')
> + for v in vals:
> + self.hintline[v] = "Ignore"
> + elif tag == "AuthorLine":
> + vals = val.split(',')
> + for v in vals:
> + self.hintline[v] = "Author"
> + elif tag == "VerseOrder":
> + self.hint_verseOrder = val
> + elif tag == "ManualCheck":
> + self.checks_needed = True
> + elif tag == "IgnoreIndent":
> + self.hint_ignoreIndent = True
> + elif tag == "VariantVerse":
> + vvline = val.split(' ', 1)
> + self.hintline[vvline[0].strip()] = "VariantVerse " + vvline[1].strip()
> + elif tag == "SongTitle":
> + self.hint_songtitle = val
> + elif tag == "AddComment":
> + self.hint_comments += '\n' + val
> + else:
> + self.log_error(file.name, "Unknown tag " + tag + " value " + val)
string formatting here please
> + return hintfound
--
https://code.launchpad.net/~john+ubuntu-g/openlp/singingthefaith/+merge/369490
Your team OpenLP Core is subscribed to branch lp:openlp.
References
