Review: Needs Fixing
Generally it looks good to me, though I haven't tested. But a few things to fix.
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-07-12
> 14:37:32 +0000
> @@ -0,0 +1,350 @@
> +# -*- 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 needs to be updated to the GPL3 license, which is also used in some of the
files you added.
> +#
> #
> +# 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
> + hint_line = {}
> + hint_file_version = '0'
> + hint_verse_order = ''
> + hint_song_title = ''
> + hint_comments = ''
> + hint_ignore_indent = False
I would propose to put the initialization of these variables in the constructor
(__init__) and make them member variables (eg. self.hints_available), otherwise
the value will not be reset the second time you run the importer.
> +
> + 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
> + if isinstance(file_path, str):
> + song_file = open(file_path, 'rt', encoding='cp1251')
> + self.do_import_file(song_file)
> + song_file.close()
> + else:
> + 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.
> + """
> + singing_the_faith_version = 1
> + 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(singing_the_faith_version))
> +
> + # 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.hint_line:
> + hint = self.hint_line[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('/')
> + new_verse = new_verse.replace(source_str,
> dest_str)
> + self.add_verse(new_verse, 'v')
> +
> self.verse_order_list.append('v{}'.format(str(current_verse_number)))
> + current_verse_number += 1
> + line_number += 1
> + next(file)
> + continue
> + else:
> +
> self.log_error(translate('SongsPlugin.SingingTheFaithImport',
> + 'File
> {file})'.format(file=file.name)),
> +
> translate('SongsPlugin.SingingTheFaithImport',
> + 'Unknown hint
> {hint}').format(hint=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_ignore_indent:
> + 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
> + elif indent == 0:
> + # If the indent is 0 and it contains '(c)' then it is a
> Copyright line
> + if '(c)' in line:
> + copyright = line
> + continue
> + elif (line.startswith('Liturgical ') or
> line.startswith('From The ') or
> + 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 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_ignore_indent 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 {file}').format(file=file.name),
> + translate('SongsPlugin.SingingTheFaithImport',
> 'Error: {error}').format(error=e))
> + return
> +
> + if self.hint_song_title:
> + song_title = self.hint_song_title
> + self.title = '{}STF{} - {title}'.format(check_flag,
> song_number.zfill(3), title=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
Please make the comment indent the same as the code.
> + if has_chorus and not self.hint_verse_order and not
> self.checks_needed:
> + auto_verse_order_ok = False
> + # Popular case V1 C2 V2 ...
> + if self.verse_order_list: # protect against odd cases
> + 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])
I assume that this will be changed at a later iteration? But just in case, you
should not include code in the comments. Either it is needed or not needed.
> + 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_verse_order:
> + self.verse_order_list = self.hint_verse_order.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 = '{}STF{} - {title}'.format(check_flag,
> song_number.zfill(3), title=song_title)
> + if not self.finish():
> + self.log_error(file.name)
> +
> + def read_hints(self, file, song_number):
Please add a description of this method.
> + hintfound = False
> + self.hint_verse_order = ''
> + self.hint_line.clear()
> + self.hint_comments = ''
> + self.hint_song_title = ''
> + self.hint_ignore_indent = 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.hint_file_version = val
> + continue
> + if (tag == 'Hymn') and (val == song_number):
> + self.add_comment('Using hints version
> {}'.format(str(self.hint_file_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.hint_line[v] = 'Comment'
> + elif tag == 'IgnoreLine':
> + vals = val.split(',')
> + for v in vals:
> + self.hint_line[v] = 'Ignore'
> + elif tag == 'AuthorLine':
> + vals = val.split(',')
> + for v in vals:
> + self.hint_line[v] = 'Author'
> + elif tag == 'VerseOrder':
> + self.hint_verse_order = val
> + elif tag == 'ManualCheck':
> + self.checks_needed = True
> + elif tag == 'IgnoreIndent':
> + self.hint_ignore_indent = True
> + elif tag == 'VariantVerse':
> + vvline = val.split(' ', 1)
> + self.hint_line[vvline[0].strip()] = 'VariantVerse
> {}'.format(vvline[1].strip())
> + elif tag == 'SongTitle':
> + self.hint_song_title = val
> + elif tag == 'AddComment':
> + self.hint_comments += '\n' + val
> + else:
> + self.log_error(file.name, 'Unknown tag {} value
> {}'.format(tag, val))
> + return hintfound
--
https://code.launchpad.net/~john+ubuntu-g/openlp/singingthefaith/+merge/369490
Your team OpenLP Core is subscribed to branch lp:openlp.
_______________________________________________
Mailing list: https://launchpad.net/~openlp-core
Post to : openlp-core@lists.launchpad.net
Unsubscribe : https://launchpad.net/~openlp-core
More help : https://help.launchpad.net/ListHelp
