Review of your modified midi2ly

[John Mandereau]

Hi Martin,

I reviewed your changed version on midi2ly with inclusion in official
sources in mind, the patch that match your work (minus trailing
whitespaces and indentation errors).  If you want your future work on
midi2ly to be considered for committing into official sources, it is
highly recommended to send patches (instead of the full file) to this
list, although you are free to send again the full script to user-list
to make user testing easier.  You can read more about submitting patches
in out Contributor's guide.

I'm sorry to set higher requirements on code quality than the original
code, but not doing so would not arrange the bad maintenance state of
this script, and would not encourage future work on it.


> diff --git a/scripts/midi2ly.py b/scripts/midi2ly.py
> index c57c788..040dec5 100644
> --- a/scripts/midi2ly.py
> +++ b/scripts/midi2ly.py
> @@ -11,7 +11,7 @@
>  '''
>  TODO:
>      * test on weird and unquantised midi input (lily-devel)
> -    * update doc and manpage
> +    * update doc and manpage and translations

Updating translations is just a matter of updating lilypond.pot and
sending it to the Translation Project, it doesn't require changing this
script again, so you needn't change this line.  However, you could add
something like

       * move each global variable used in the conversion process to
global_options or per-file variable

(see just below)


> @@ -497,19 +505,28 @@ def events_on_channel (channel):
>                  seconds_per_1 = us_per_4 * 4 / 1e6
>                  events.append ((t, Tempo (seconds_per_1)))
>              elif e[1][1] == midi.TIME_SIGNATURE:
> +                global num, den

Global variables are evil when multiple files can be processed, better
define these as per-files variables or attributes of global_options.


>                  (num, dur, clocks4, count32) = map (ord, e[1][2])
>                  den = 2 ** dur 
> +                if global_options.mytime:
> +                   num = 
> int(global_options.mytime[:global_options.mytime.index('/')])
> +                   den = 
> int(global_options.mytime[global_options.mytime.index('/')+1:])

Could you rewrite this using something more concise and clear like

num, den = [int (s) for s in global_options.mytime.split ('/')]

after having checked at the options parsing stage that mytime has a correct 
form?


>                  events.append ((t, Time (num, den)))
>              elif e[1][1] == midi.KEY_SIGNATURE:
>                  (alterations, minor) = map (ord, e[1][2])
>                  sharps = 0
>                  flats = 0
> +
>                  if alterations < 127:
>                      sharps = alterations
>                  else:
>                      flats = 256 - alterations
>  
> -                k = Key (sharps, flats, minor)
> +                if mysharps == myflats:
> +                    k = Key (sharps, flats, minor)
> +                else:
> +                    k = Key (mysharps, myflats, myminor)
> +

Why do you use variable names starting with "my"?  If it means some
values can be overriden by the user, this should not appear in the
variable names but the variables should be initalized with the values
properties of global_options in case this override is forced by some
option or makes sense in all situations.


> +def mykey (sharps, flats, minor):
> +    ks = [ 'g' , 'd' , 'a' , 'e' , 'b' , 'fis' , 'cis', 'gis', 'dis' ]
> +    kf = [ 'd', 'g', 'c' , 'f' , 'bes' , 'es' , 'as' , 'des' , 'ges' , 'ces' 
> ] 
> +    if sharps > 0:
> +        mykey = ks [ sharps - 1 + 3*minor ]
> +    elif flats > 0:
> +        mykey = kf [ 2 + flats - 3*minor ]
> +    else:
> +        if minor == 1:
> +            mykey = 'a'
> +        else:
> +            mykey = 'c'

Nice, but why don't you handle the case sharps == flats == 0 by
prepending 'a' to kf, replacing "elif flats...:" with "else:" and adding
1 to kf index?  You could even have only one list named fifth_helix and
use -flats instead of +flats.

By the way:
- this list (or these two lists) should be tuples,
- this duplicates lists at the beginning of class Key,
- full names (with underscores for compound names) would be better than
initialisms like kf, ks;
- we use no space at the inside side of brackets and parentheses, e.g.
"[sharps - 1]" and not "[ sharps -1 ]", and list and dictionary
subscripts should not be separated by space, e.g.
"ks[sharps - 1]" not "ks [sharps - 1]".

Oh, and did you notice your function duplicates the functionality of
Key.dump?  Please consider replacing Key.dump with your function, which
I find clearer.

 
>  def convert_midi (in_file, out_file):
>      global clocks_per_1, clocks_per_4, key
>      global start_quant_clocks
> -    global  duration_quant_clocks
> +    global duration_quant_clocks
>      global allowed_tuplet_clocks
> +    global mysharps, myflats, myminor, use_mykey

Same thing as above: prefer global options, or variables specific to
each file, over global variables.



>      str = open (in_file).read ()
>      midi_dump = midi.parse (str)
> 
> @@ -804,33 +848,63 @@ def convert_midi (in_file, out_file):
>      for (dur, num, den) in global_options.allowed_tuplets:
>          allowed_tuplet_clocks.append (clocks_per_1 / den)
>  
> +    if global_options.key.sharps + global_options.key.flats + 
> global_options.key.minor !=0:
> 
> +        mysharps = global_options.key.sharps
> +        myflats = global_options.key.flats
> +        myminor = global_options.key.minor
> +        use_mykey = 1
> +    else:
> +        mysharps = global_options.key.sharps
> +        myflats = global_options.key.flats
> +        myminor = global_options.key.minor
> +        use_mykey = 0

I don't see the point of defining global variables mysharps, myflats,
myminor if you can retrieve their values as well through global_options;
use_mykey should be defined as an attribute of global_options.


> +    s = s + '\\header {\n'

"s +=" is as clear but shorter.


>      i = 0
> +    A = 0

Could A be a boolean and be given a more explicit name?

> +    p.add_option ('-P', '--partial',
> +           metavar =_ ("DUR"),
> +           action = "store",
> +           dest = "mypartial",
> +           help =_ ("insert global \"\\partial DUR\" upbeat"))
> +    p.add_option('-s', '--start-quant',
> +           help= _ ("quantise note starts on DUR"),
> +           default = '16',
> +           metavar= _ ("DUR"))
> +    p.add_option ('--skip',
> +           action = "store_true",
> +           help =_ ("use s instead of r for rests"))
> +    p.add_option('-t', '--allow-tuplet',
>             metavar=_ ("DUR*NUM/DEN"),
>             action = "append",
>             dest="allowed_tuplets",
> -           help=_ ("allow tuplet durations DUR*NUM/DEN"),
> +           help= _ ("allow tuplet durations DUR*NUM/DEN"),
>             default=[])
> +    p.add_option ('-T', '--time',
> +           metavar =_ ("NUM/DEN"),
> +           action = "store",
> +           dest = "mytime",
> +           help =_ ("insert global \"\\time NUM/DEN\" signature"))
>      p.add_option ('-V', '--verbose', help=_ ("be verbose"),
> -           action='store_true'
> -           ),
> -    p.version = "midi2ly (LilyPond) @TOPLEVEL_VERSION@"
> +           action='store_true')
> +    p.version = "midi2ly (LilyPond) %s" % ( program_version )

Spaces usage is incorrect in some places and not consistent with
previous spacing in some other places.  I don't see why variable names
are prefixed with "my" (as above).


> @@ -913,11 +1007,8 @@ def do_options ():
>      if options.duration_quant:
>          options.duration_quant = int (options.duration_quant)
>  
> -    if options.warranty:
> -        warranty ()
> -        sys.exit (0)
>      if 1:
> -        (alterations, minor) = map (int, (options.key + ':0').split 
> (':'))[0:2]
> +        (alterations, minor) = map (int, (options.mykey + ':0').split 
> (':'))[0:2]

Again, I'm not fond prefixing variable names with "my".

I'm looking forward to a new patch against current master branch (or
equally 2.13.6 sources) that solves these nitpicks.

Best,
John

midi2ly-Martin-Tarenskeen.patch
Description: Text Data

signature.asc
Description: Ceci est une partie de message numériquement signée