Re: [PATCHES]: Converting header and lyrics with musicxml2ly

[Han-Wen Nienhuys]

Reinhold Kainhofer escreveu:
> Attached is the next set of patches for musicxml2ly, diffed against current 
> git by git-format-patch.

Hi, 

Thanks for your patches!

if you intend to work on this more, I could give you write access to the 
repository.  It would require you to sign up for savannah.gnu.org.

I am traveling the next 2 weeks, so I am won't have much time to look/
I'm applying your patches. Could you address my comments in follow up 
patches?

* In general, git commit messages should have the following form

  1-line summary

  background - typically the reason why you are doing it, or the 
  general idea behind it.

Keep the 80 column limit in mind here. Perhaps it's worthwhile to 
open the 1-line summary with 'MusicXML: '

* You have replaced almost all dict accesses with .get() ; this is only
desired if the default case makes sense. Sometime throwing a KeyError 
actually does make sense.

For example

-        (glyph, pos, c0) = self.clef_dict [self.type]
+        (glyph, pos, c0) = self.clef_dict.get (self.type)

doesn't really make sense, because the assignment will fail the default,
None.


If you have to check for the default value

  if key in dict:
    ..do stuff..
  
is clearer then

  foo = dict.get(key, None)
  if foo:
    ...

* Also fix problem that \breathe does not accept any direction modifyer

modifier


    # return the first creator tag that has type 'editor'
+        for i in creators:
+            if hasattr (i, 'type') and i.type == type:
+                return i.get_text ()
+            else:
+                return ''

I don't understand the comment, do you mean "type" iso. editor?

+        if ids.get_rights ():
+            score_information['copyright'] = ids.get_rights ()

you could shorten this a little, 

  def set_if_exist(key, val):
     if val: 
        score_information[key] = val
  set_if_exists ('copyright', ids.get_rights ())

removing the duplicate call of ids.get_XXX()

+        printer.dump ('%s = "%s"' % (k, score_information[k]))

this probably needs some escaping. What if there is a " in the title?

+    for k in score_information.keys ():

slightly simpler: 

  for (k, v) in score_information.items():
    printer.dump ('%s = "%s"' % (k, v))



-- 

Han-Wen Nienhuys - address@hidden - http://www.xs4all.nl/~hanwen