sinshu / go-meltysynth

A SoundFont MIDI synthesizer written in pure Golang
Other
62 stars 11 forks source link

meltysynth: make main and tests more robust #1

Closed kardianos closed 1 year ago

kardianos commented 1 year ago

Remove hardcoded sf2 and midi files in main and test files. Move test files to meltysynth folder. Remove extra white space. Simplify some variable declarations, loops, and if statements.

sinshu commented 1 year ago

Thank you very much 😄

I am still new to Go, so your modifications to make the code more Go-like are very helpful.

I have some questions about this PR:

kardianos commented 1 year ago
  • Is it common in Go to put normal code and test code in the same directory? If so, that is fine.

Yes, you put testing code in the same directory. The "_test" suffix at the end of the filename is recognized by the build system and creates an implicit build tag that is ignored during normal build, so test code, even when part of the same package, is not built or included in a final binary or package.

There are two package names you can use. To test internal methods, the test package name should be the same as the package you are trying to test. Sometimes you want to test external functions only and you have the option of naming it _test which will compile it lexical as a different package.

  • Is it important to eliminate hard-coded SF2 paths in testing? The current tests are already a mass of hard-coded code tied to specific SF2s, and I don't think there is much benefit in avoiding only hard-coded paths.

It is important that testing code, if the SF2 file is not found, to be Skipped gracefully with an appropriate message. I'll modify the code so it looks for those files by default, allows a env override, and still skips if not present. Wait for the next drop.

kardianos commented 1 year ago

I just updated the testing code to look for the default filename, and if present, use them, but if not, skip the test. It also still looks for an env variable if the sf2 files are present elsewhere on the FS.

kardianos commented 1 year ago

Oh, there is one question: there is a variable named "halfPi in soundfont_math.go, but it is just set to math.Pi (which is a const, not a float32 or float64. Should that be set to math.Pi / 2? Unsure.

sinshu commented 1 year ago

Regarding halfPi, that should be a bug. I'll fix it later.

sinshu commented 1 year ago

I merged the PR. Thank you very much 😄