OpenLP

Merge lp://qastaging/~john+ubuntu-g/openlp/singingthefaith into lp://qastaging/openlp

  1. singingthefaith
  2. Merge into trunk
Proposed by John Lines
Status: Superseded
Proposed branch: lp://qastaging/~john+ubuntu-g/openlp/singingthefaith
Merge into: lp://qastaging/openlp
Diff against target: 585 lines (+501/-11)
8 files modified
openlp/plugins/songs/lib/importer.py (+22/-10)
openlp/plugins/songs/lib/importers/singingthefaith.py (+347/-0)
tests/functional/openlp_plugins/songs/test_singingthefaithimport.py (+50/-0)
tests/helpers/songfileimport.py (+1/-1)
tests/resources/songs/singingthefaith/H1.txt (+9/-0)
tests/resources/songs/singingthefaith/H2.txt (+30/-0)
tests/resources/songs/singingthefaith/STF001.json (+13/-0)
tests/resources/songs/singingthefaith/STF002.json (+29/-0)
To merge this branch: bzr merge lp://qastaging/~john+ubuntu-g/openlp/singingthefaith
Reviewer Review Type Date Requested Status
Raoul Snyman Needs Fixing
Review via email: mp+369398@code.qastaging.launchpad.net

This proposal has been superseded by a proposal from 2019-06-30.

Commit message

Initial merge of SingingTheFaithImport, including update to importer.py

Description of the change

Singing The Faith is the new Authorized Hymn book for the Methodist Church of Great Britain.
There is an electronic version of the Hymn book, for Windows only, which can export Hymns as text files.

This import module smooths the process of converting these text files into OpenLP. The input format is messy and not intended for automatic processing so the importer uses a combination of heuristics and a hints file. This version has not been tested on all the hymns in Singing The Faith, but deals with most of the, more than 100, hymns it has been tested with.

Note that it includes a test module, but that test module fails, and I am not sure why.

To post a comment you must log in.
Revision history for this message
Raoul Snyman (raoul-snyman) wrote :

Linux tests failed, please see https://ci.openlp.io/job/MP-02-Linux_Tests/191/ for more details

Revision history for this message
Raoul Snyman (raoul-snyman) wrote :

The test is failing because the "add_verse" method is not called, or not called with that exact data. I've broken up the error message into 3 parts below for easier reading.

AssertionError:

add_verse('Amazing grace! How sweet the sound!\nThat saved a wretch like me!\nI once was lost, but now am found;\nWas blind, but now I see.', 'v1')

call not found

review: Needs Fixing
Revision history for this message
Phill (phill-ridout) wrote :

A few in-line comments. Also you don't need to use parenthesis around the expressions in the if statements.

lp://qastaging/~john+ubuntu-g/openlp/singingthefaith updated
2884. By john <email address hidden>

Use path objects and remove redundant brackets

2885. By john <email address hidden>

Test fixed for single verse, all verses failing test not run yet

2886. By john <email address hidden>

Merge trunk updates

2887. By john <email address hidden>

Fix undefined auto_verse_order_ok case, and add some test resources

Revision history for this message
John Lines (john+ubuntu-g) wrote :

> A few in-line comments. Also you don't need to use parenthesis around the
> expressions in the if statements.

Thanks - have updated to use Path more, and have removed redudant parentheses round expressions in if statements

Revision history for this message
Raoul Snyman (raoul-snyman) wrote :

Hey John, a couple things.

1. You have a ton of linting issues. I started commenting, and then I realised that it would be better to just point you to flake8. See the link at the bottom of my comment for a quick introduction to linting and flake8.

2. You have some inconsistent indentation. Indentation in Python is very important, so we take it seriously. Also, please make sure you are indenting using spaces and not tabs.

3. You are not committing your code with the e-mail address associated with Launchpad. Please can you fix that by issuing a bzr whoami "John Lines <email address hidden>"

4. Once you've made all your changes, and you're ready for another review, you need to resubmit your merge proposal. Do this by clicking the "Resubmit" link in the top right hand corner of the page.

5. If you're struggling with anything, pop into our IRC channel, there's usually someone around who is happy to help.

https://medium.com/python-pandemonium/what-is-flake8-and-why-we-should-use-it-b89bd78073f2

review: Needs Fixing
lp://qastaging/~john+ubuntu-g/openlp/singingthefaith updated
2888. By John Lines

Fix flake8 warnings, print statements, inline comments

Revision history for this message
John Lines (john+ubuntu-g) wrote :

1. Thanks for the info on flake8 - is now flake8 clean - you are quite right - it is better for me to learn about the tools.
2. Think indentation is fixed - flake8 was handy as well
3. Email address now set in my bzr config
4. Can you have another look at the change now.

lp://qastaging/~john+ubuntu-g/openlp/singingthefaith updated
2889. By John Lines

Fix lint warning in tests and importer.py

2890. By John Lines

Minimize differences needed to run under OpenLP 2.4.6

2891. By John Lines

Convert double to single quotes, make hint variable names consistent

2892. By John Lines

New style string formatting, restructure indent=0 cases

2893. By John Lines

New hints for AddSpaceAfterSemi and CCLI

2894. By John Lines

Update GPL version, use constructor, add comment to signal unepected verse order

2895. By John Lines

Add AddSpaceAfterColon hint

2896. By John Lines

Add BlankLine hint and deal with a leading asterisk on a verse number

2897. By John Lines

Merge trunk updates

2898. By John Lines

enable whole song impoerter test

2899. By John Lines

Strip unwanted formatting characters

2900. By John Lines

Implement BoldLine hint

2901. By John Lines

Make Based on Psalm an automatic comment, do not automatically make Authors type Word

2902. By John Lines

Fix typo

2903. By John Lines

Merge trunk updates

2904. By John Lines

Use .format in importer.py

2905. By John Lines

Implement SongbookNumberInTitle hint

2906. By John Lines

Upload hints.tag into resources

2907. By John Lines

fix tests to allow for SongbookNumberInTitle defaulting to false and add hints tests

2908. By John Lines

Merge trunk updates

2909. By John Lines

add tests with hints subdirectory

2910. By John Lines

Linting fix

2911. By John Lines

put default hints plugin directory and chnage name to singingthefaith-hints.tag

2912. By John Lines

Merge trunk updates

2913. By John Lines

Tweaks to hints - now version 3, H470 and H567

Unmerged revisions

2913. By John Lines

Tweaks to hints - now version 3, H470 and H567

2912. By John Lines

Merge trunk updates

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The diff is not available at this time. You can reload the page or download it.