Re: musicxml2ly: title and subtitle (issue 1913), miscellaneous (issue 5096050)

[ptrcklschmdt]

On 2011/09/22 13:26:54, Reinhold wrote:
> 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:

> address@hidden:~$ cat .bin/musicxml2ly
> #!/bin/sh
> exec ~/lilypond/lilypond/out/bin/musicxml2ly "$@"

> You can then simply call musicxml2ly, and the built musicxml2ly will
be called.

OK, I basically use the same method, but the "make-trick" was new to me.
I obviously modified the program files directly in the build directory
and copied them to the source tree because 'git status' couldn't find
any modified files.
> 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.

OK
> 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.

Hm, I didn't understand all of this. What do I have to change in
musicxml2ly? My idea was to use the "description" misc field for a
custom header variable named 'miscellaneous' by default. I was thinking
about implementing a cmd line option ('-t' and '--texinfo') to be able
to use the 'texinfo' variable when needed. But if at all this should be
implemented in a different patch...
> 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

OK

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.

OK

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.

See above

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..
OK

@Janek: I'll add some comments in the next patch.

Thank you both for your comments!

http://codereview.appspot.com/5096050/