Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(609)

Issue 573230043: musicxml: Use method to get number of Lyric

Can't Edit
Can't Publish+Mail
Start Review
Created:
1 month ago by hahnjo
Modified:
1 month ago
Reviewers:
Dan Eble
CC:
lilypond-devel_gnu.org
Visibility:
Public.

Description

musicxml: Use method to get number of Lyric Commit edb29ef3a ("Correct an inconsistency in the Lyric class") renamed the method from get_number() to number(), which is shadowed by the attribute itself and not used in the current code. Using a method has the advantage that the code can cast to int() in a single place which is needed for Python 3. Additionally, the code handles the case if there is no 'number' attribute (should this ever happen).

Patch Set 1 #

Total comments: 2
Unified diffs Side-by-side diffs Delta from patch set Stats (+7 lines, -7 lines) Patch
M python/musicxml.py View 3 chunks +3 lines, -3 lines 2 comments Download
M scripts/musicxml2ly.py View 2 chunks +4 lines, -4 lines 0 comments Download

Messages

Total messages: 2
Dan Eble
LGTM https://codereview.appspot.com/573230043/diff/559230043/python/musicxml.py File python/musicxml.py (right): https://codereview.appspot.com/573230043/diff/559230043/python/musicxml.py#newcode1243 python/musicxml.py:1243: nr = l.get_number() I infer from your change ...
1 month ago (2019-11-11 13:34:39 UTC) #1
hahnjo
1 month ago (2019-11-11 15:50:42 UTC) #2
https://codereview.appspot.com/573230043/diff/559230043/python/musicxml.py
File python/musicxml.py (right):

https://codereview.appspot.com/573230043/diff/559230043/python/musicxml.py#ne...
python/musicxml.py:1243: nr = l.get_number()
On 2019/11/11 13:34:38, Dan Eble wrote:
> I infer from your change summary that you verified that this is called at some
> point during the regression tests.  Is that correct?

Yes, it originally showed up when porting to Python 3 (I'm currently just
dividing up the resulting diff). To be sure I just verified that adding a
sys.exit(1) instead of this line breaks the tests, so I think this code path is
indeed exercised by at least a few regression tests.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b