Welcome to the wonderful world of LilyPond development! Most changes look fine, but there are some things that can't be submitted yet. See my comments.
Most important: Please edit the source file in scripts/musicxml2ly.py and don't modify the installed musicxml2ly file and copy it over to the source tree! In particular, the source code contains identifiers like @TOPLEVEL_VERSION@, which will be replaced by the build system with proper values, so that we don't have to hardcode things like version or python path! So, -) Modify only the scripts/musicxml2ly.py and python/musicexp.py and python/musicxml.py -) Run make (you can kill it after a few seconds, as soon as all python files are processed, which happends right at the beginning) -) You can't run scripts/musicxml2ly.py directly, but rather the built out/bin/musicxml2ly For the last item, on my computer, I have created a simply wrapper script ~/.bin/musicxml2ly (if ~/.bin is in your PATH environment variable), which contains only: reinhold@curie:~$ cat .bin/musicxml2ly #!/bin/sh exec ~/lilypond/lilypond/out/bin/musicxml2ly "$@" You can then simply call musicxml2ly, and the built musicxml2ly will be called. http://codereview.appspot.com/5096050/diff/1/python/musicexp.py File python/musicexp.py (right): http://codereview.appspot.com/5096050/diff/1/python/musicexp.py#newcode63 python/musicexp.py:63: self.print_verbatim ('\\version "2.15.13"') On 2011/09/22 12:50:43, janek wrote:
Isn't this a mistake?
I suppose it is a mistake. The source should contain @TOPLEVEL_VERSION@, which will be replaced by the current version by make. http://codereview.appspot.com/5096050/diff/1/python/musicexp.py#newcode283 python/musicexp.py:283: return False These functions should not be placed here. The pitch language functions are here, because the Pitch class is next. The midi functions don't belong here. http://codereview.appspot.com/5096050/diff/1/python/musicxml.py File python/musicxml.py (right): http://codereview.appspot.com/5096050/diff/1/python/musicxml.py#newcode252 python/musicxml.py:252: return None Please don't exactly duplicate get_file_description! A much cleaner solution would be to rename get_file_description to get_miscellaneous and return a hash of all miscellaneous fields (note that the 'name' attribute is REQUIRED in the MusicXML specification)... Something like: def get_miscellaneous (self): misc = self.get_named_children ('miscellaneous') ret = [] for m in misc: misc_fields = m.get_named_children ('miscellaneous-field') for mf in misc_fields: if hasattr (mf, 'name'): ret[mf.name] = mf.get_text () else: // Print a warning here... return ret Instead of the if hasattr you can also use mf.get('name', ''). Of course, you'll have to adjust musicxml2ly.py, too. But at least this solution is more general, and the logic to abuse a "description" misc field for the texidoc is implemented in the main file, not in a library file. http://codereview.appspot.com/5096050/diff/1/scripts/musicxml2ly.py File scripts/musicxml2ly.py (right): http://codereview.appspot.com/5096050/diff/1/scripts/musicxml2ly.py#newcode1 scripts/musicxml2ly.py:1: #!/usr/bin/python Please don't modify the compiled musicxml2ly and copy it to the source tree. Rather modify the scripts/musicxml2ly.py in the source tree and call make. To run it, simply call out/bin/musicxml2ly. The source code MUST have the @TARGET_PYTHON@, @relocate-preamble@, etc. http://codereview.appspot.com/5096050/diff/1/scripts/musicxml2ly.py#newcode33 scripts/musicxml2ly.py:33: """ Same as above: Needs to be @relocate-preamble@ in the code! http://codereview.appspot.com/5096050/diff/1/scripts/musicxml2ly.py#newcode210 scripts/musicxml2ly.py:210: set_if_exists ('subtitle', movement_title.get_text ()) "else" is missing (if there is no work, then no title will be set at all, even if movement_title exists). I would structure the if as follows (pseudocode): if work: 'title' = work_title if movement_title: 'subtitle' = movement_title elif movement_title: 'title' = movement_title http://codereview.appspot.com/5096050/diff/1/scripts/musicxml2ly.py#newcode221 scripts/musicxml2ly.py:221: # set_if_exists ('tagline', ids.get_encoding_software ()) Simply remove the code without comment. http://codereview.appspot.com/5096050/diff/1/scripts/musicxml2ly.py#newcode231 scripts/musicxml2ly.py:231: # set_if_exists ('miscellaneous', ids.get_miscellaneous ()); If you change get_miscellaneous to a hash, you can extract the 'description' here for the texidoc, and loop through all fields to insert custom header fields for them. http://codereview.appspot.com/5096050/diff/1/scripts/musicxml2ly.py#newcode2596 scripts/musicxml2ly.py:2596: p.version = ('''%prog (LilyPond) 2.15.13\n\n''' @TOPLEVEL_VERSION@ http://codereview.appspot.com/5096050/diff/1/scripts/musicxml2ly.py#newcode2797 scripts/musicxml2ly.py:2797: printer.print_verbatim ('%% automatically converted with musicxml2ly from %s\n' % filename) To me "converted by musicxml2ly" sounds better.. http://codereview.appspot.com/5096050/ _______________________________________________ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
