nwhitehead / pyfluidsynth

Python bindings for FluidSynth
GNU Lesser General Public License v2.1
197 stars 56 forks source link

Changed argument naming to better match fluidsynth #29

Closed Avril112113 closed 3 years ago

Avril112113 commented 3 years ago

I found the naming of certain arguments related to presets rather confusing and when I looked up the naming in the fluidsynth header files, I found them a lot more correct.

This PR renames uses of prognum to prenum and presetnum The reason for using 2 different names is to keep the naming similar to related other variable names I did not change info.program at line 615 in fluidsynth.py because the naming was correct to the header and documentation of fluidsynth

Relevent Sources: Line 95: http://www.nongnu.org/fluid/api/sfont_8h-source.html Line 139: http://www.nongnu.org/fluid/api/synth_8h-source.html

I am completley fine with any reason to not merge this PR, but I thought these changes would be helpful and reduce confusion.

ChristianRomberg commented 3 years ago

I am completley fine with any reason to not merge this PR, but I thought these changes would be helpful and reduce confusion.

I'm hard pressed to come up with a reason to not merge this. I think that added consistency is beneficial to this library. Thank you for your contribution.

@albedozero Should we push the version number on this one? I'm not sure since it's such a minor change but it could technically break existing code (because a function parameter has been renamed).

albedozero commented 3 years ago

I'm fine with merging too - the misnaming of presets as programs has been in the back of my mind for a while. Would it make sense to go even further and replace every prenum or presetnum with simply preset? I appreciate attempting to stay consistent @Dude112113, but I don't think the multiple people who've added to the code (including me) had a consistent plan to begin with, so simpler is probably better. Also, agree @ChristianRomberg technically it should require a version bump, but since none of the functions involved actually use keyword arguments I don't think it would actually break code.

Avril112113 commented 3 years ago

I could swap the variable naming so line 640 uses prenum since it's never used, and use presetnum for the rest. I concern for clashing variable names is the reason why they are different, and I should have done it this way around to begin with. Besides that the only other occurence of prenum is in fluid_sfont_get_preset arguments which refers to the DLL, which the arg name is prenum in the header file. I am happy to change that for you. Every other occurence presetnum was used.

PS: I know the feeling about figuring out github, I am still learning it myself.

albedozero commented 3 years ago

Ah, I see what you're saying, and you're right - using presetnum as a function parameter in 631 and argument 635 would cause line 640 to reassign that variable to the output of the channel_info function. This wouldn't break anything, but it's bad practice since the variable in 640 is for a different purpose. I also see now that I mistakenly passed prognum as the third argument for the fluid_sfont_get_preset prototype in line 483 back when I added the sfpreset_name function, since it gets called that way several times in _fluidsynth.c. It doesn't have to match the header, it just sets the name of the parameter in the python callable - but it makes more sense to match what's in the header and it is, in fact, referring to a preset. I'll go ahead and merge this, since it makes clearer what things are presets and it's cleaner code. Thanks!