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

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

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

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

> 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

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

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.

Linux tests passed!

Linting failed, please see https://ci.openlp.io/job/MP-03-Linting/132/ for more details

Fix lint tests outside main importer code

Linux tests passed!

Linting passed!

macOS tests passed!

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?

Generally it looks good to me, though I haven't tested. But a few things to fix.

Using new string formatting, constructor for class.

Linux tests passed!

Linting passed!

Just a few more inline comments.

Hey John, if you're ready for us to take another look at this, make sure to resubmit it.

Is there any reason the hint file isn't included? If it is not bundled with OpenLP, then where is the user supposed to get it from?

The use of STFnnn - in the title is now controlled by a hint. Also other code tidying.

Hi John,

I still don't get why the hint file is not included...
If you already made the file, why should others have to do it? I assume that the "Singing The Faith" files that can be imported are the same for all potential users?