openlp-core team mailing list archive
-
openlp-core team
-
Mailing list archive
-
Message #25537
Re: [Merge] lp:~alisonken1/openlp/ccli-usr-bin-fix into lp:openlp
Clarification: Looking at the loop, the question I have is "why is the
second check out of 6 checks inconsistent with the control structure?"
That whole section is predicated on checking the beginning of a line.
If the beginning of the line has already been matched with the first
"if", why is it being rechecked for a different value?
Original control statement:
for line in text_list:
if line.startswith('[S '):
...
if line.startswith('Title='):
...
elif line.startswith('Author='):
...
elif line.startswith('Copyright='):
...
elif line.startswith('Themes='):
...
elif line.startswith('Fields='):
...
elif line.startswith('Words='):
...
# Unhandled usr keywords: Type, Version, Admin, Keys
When looking at code like that, it gives the impression that someone
is not paying attention to their control loop.
On Sat, Jan 10, 2015 at 8:04 PM, Ken Roberts <alisonken1@xxxxxxxxx> wrote:
> Is there a reason why the original 'if' statement on the second check
> is not consistent with the purpose of the rest of the checks?
>
> On Sat, Jan 10, 2015 at 11:40 AM, Tim Bentley <tim.bentley@xxxxxxxxx> wrote:
>> Review: Needs Fixing
>>
>>
<snip>
>>
>>> ``[File]``
>>> USR file format first line
>>> @@ -160,7 +163,7 @@
>>> self.ccli_number = ccli[4:].strip()
>>> else:
>>> self.ccli_number = ccli[3:].strip()
>>> - if line.startswith('Title='):
>>
>> Why change when the previous if can change the value of line.
>>
>>> + elif line.startswith('Title='):
>>> self.title = line[6:].strip()
>>> elif line.startswith('Author='):
>>> song_author = line[7:].strip()
<snip>
--
- Ken
Registered Linux user 296561
Slackin' since 1993
Slackware Linux (http://www.slackware.com)
https://code.launchpad.net/~alisonken1/openlp/ccli-usr-bin-fix/+merge/246037
Your team OpenLP Core is subscribed to branch lp:openlp.
References