The proposal to merge lp:~john+ubuntu-g/openlp/singingthefaith into lp:openlp
has been updated.
Status: Approved => Merged
For more details, see:
https://code.launchpad.net/~john+ubuntu-g/openlp/singingthefaith/+merge/372277
--
Your team OpenLP Core is subscribed to branch lp:openlp.
The proposal to merge lp:~john+ubuntu-g/openlp/singingthefaith into lp:openlp
has been updated.
Status: Needs review => Approved
For more details, see:
https://code.launchpad.net/~john+ubuntu-g/openlp/singingthefaith/+merge/372277
--
Your team OpenLP Core is subscribed to branch lp:openlp.
Review: Approve
--
https://code.launchpad.net/~john+ubuntu-g/openlp/singingthefaith/+merge/372277
Your team OpenLP Core is subscribed to branch lp:openlp.
___
Mailing list: https://launchpad.net/~openlp-core
Post to :
Review: Approve
Looks good.
--
https://code.launchpad.net/~john+ubuntu-g/openlp/singingthefaith/+merge/372277
Your team OpenLP Core is subscribed to branch lp:openlp.
___
Mailing list: https://launchpad.net/~openlp-core
Post to :
John Lines has proposed merging lp:~john+ubuntu-g/openlp/singingthefaith into
lp:openlp.
Commit message:
Initial merge of SingingTheFaithImport, including update to importer.py
Requested reviews:
Tomas Groth (tomasgroth)
Phill (phill-ridout)
Raoul Snyman (raoul-snyman)
For more details,
The proposal to merge lp:~john+ubuntu-g/openlp/singingthefaith into lp:openlp
has been updated.
Status: Needs review => Superseded
For more details, see:
https://code.launchpad.net/~john+ubuntu-g/openlp/singingthefaith/+merge/372194
--
Your team OpenLP Core is subscribed to branch
Review: Needs Fixing
Hi John, sorry for mentioning this before, but the hint file should be placed
in the folder openlp/plugins/songs/lib/importers/
If the user does not supply a hint file you can then load it from this path:
AppLocation.get_directory(AppLocation.PluginsDir) / 'songs' / 'lib' /
John Lines has proposed merging lp:~john+ubuntu-g/openlp/singingthefaith into
lp:openlp.
Commit message:
Initial merge of SingingTheFaithImport, including update to importer.py
Requested reviews:
Raoul Snyman (raoul-snyman)
Tomas Groth (tomasgroth)
Phill (phill-ridout)
For more details,
The proposal to merge lp:~john+ubuntu-g/openlp/singingthefaith into lp:openlp
has been updated.
Status: Needs review => Superseded
For more details, see:
https://code.launchpad.net/~john+ubuntu-g/openlp/singingthefaith/+merge/372191
--
Your team OpenLP Core is subscribed to branch
John Lines has proposed merging lp:~john+ubuntu-g/openlp/singingthefaith into
lp:openlp.
Commit message:
Initial merge of SingingTheFaithImport, including update to importer.py
Requested reviews:
Tomas Groth (tomasgroth)
Phill (phill-ridout)
Raoul Snyman (raoul-snyman)
For more details,
The proposal to merge lp:~john+ubuntu-g/openlp/singingthefaith into lp:openlp
has been updated.
Status: Needs review => Superseded
For more details, see:
https://code.launchpad.net/~john+ubuntu-g/openlp/singingthefaith/+merge/372031
--
Your team OpenLP Core is subscribed to branch
Review: Needs Fixing
The tests are failing, see
https://ci.openlp.io/job/MP-03-Linux-Tests/234/console
Also see the inline comment.
Diff comments:
>
> === added file 'resources/hints.tag'
> --- resources/hints.tag 1970-01-01 00:00:00 +
> +++ resources/hints.tag 2019-08-29
The proposal to merge lp:~john+ubuntu-g/openlp/singingthefaith into lp:openlp
has been updated.
Status: Needs review => Superseded
For more details, see:
https://code.launchpad.net/~john+ubuntu-g/openlp/singingthefaith/+merge/370364
--
Your team OpenLP Core is subscribed to branch
Review: Needs Information
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?
--
The use of STFnnn - in the title is now controlled by a hint. Also other code
tidying.
--
https://code.launchpad.net/~john+ubuntu-g/openlp/singingthefaith/+merge/370364
Your team OpenLP Core is subscribed to branch lp:openlp.
___
Mailing list:
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?
--
https://code.launchpad.net/~john+ubuntu-g/openlp/singingthefaith/+merge/370364
Your team OpenLP Core is subscribed to branch lp:openlp.
Hey John, if you're ready for us to take another look at this, make sure to
resubmit it.
--
https://code.launchpad.net/~john+ubuntu-g/openlp/singingthefaith/+merge/370364
Your team OpenLP Core is subscribed to branch lp:openlp.
___
Mailing list:
Just a few more inline comments.
Diff comments:
> === modified file 'openlp/plugins/songs/lib/importer.py'
> --- openlp/plugins/songs/lib/importer.py 2019-04-13 13:00:22 +
> +++ openlp/plugins/songs/lib/importer.py 2019-07-28 08:47:32 +
> @@ -343,6 +345,15 @@
>
The proposal to merge lp:~john+ubuntu-g/openlp/singingthefaith into lp:openlp
has been updated.
Status: Needs review => Superseded
For more details, see:
https://code.launchpad.net/~john+ubuntu-g/openlp/singingthefaith/+merge/369490
--
Your team OpenLP Core is subscribed to branch
Using new string formatting, constructor for class.
--
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
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 +
> +++
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
John Lines has proposed merging lp:~john+ubuntu-g/openlp/singingthefaith into
lp:openlp.
Commit message:
Initial merge of SingingTheFaithImport, including update to importer.py
Requested reviews:
Raoul Snyman (raoul-snyman)
For more details, see:
The proposal to merge lp:~john+ubuntu-g/openlp/singingthefaith into lp:openlp
has been updated.
Status: Needs review => Superseded
For more details, see:
https://code.launchpad.net/~john+ubuntu-g/openlp/singingthefaith/+merge/369489
--
Your team OpenLP Core is subscribed to branch
Fix lint tests outside main importer code
--
https://code.launchpad.net/~john+ubuntu-g/openlp/singingthefaith/+merge/369489
Your team OpenLP Core is subscribed to branch lp:openlp.
___
Mailing list: https://launchpad.net/~openlp-core
Post to :
John Lines has proposed merging lp:~john+ubuntu-g/openlp/singingthefaith into
lp:openlp.
Commit message:
Initial merge of SingingTheFaithImport, including update to importer.py
Requested reviews:
Raoul Snyman (raoul-snyman)
For more details, see:
The proposal to merge lp:~john+ubuntu-g/openlp/singingthefaith into lp:openlp
has been updated.
Status: Needs review => Superseded
For more details, see:
https://code.launchpad.net/~john+ubuntu-g/openlp/singingthefaith/+merge/369398
--
Your team OpenLP Core is subscribed to branch
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.
--
Review: Needs Fixing
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
> 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
--
A few in-line comments. Also you don't need to use parenthesis around the
expressions in the if statements.
Diff comments:
>
> === added file 'openlp/plugins/songs/lib/importers/singingthefaith.py'
> --- openlp/plugins/songs/lib/importers/singingthefaith.py 1970-01-01
> 00:00:00 +
>
Review: Needs Fixing
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
John Lines has proposed merging lp:~john+ubuntu-g/openlp/singingthefaith into
lp:openlp.
Commit message:
Initial merge of SingingTheFaithImport, including update to importer.py
Requested reviews:
OpenLP Core (openlp-core)
For more details, see:
33 matches
Mail list logo
