search

liamappelbe / wav

Dart package for reading and writing wav files
Apache License 2.0
18 stars 5 forks source link

Separate reader and writer classes #9

Closed jamescrosswell closed 1 year ago

jamescrosswell commented 1 year ago

Summary

The original code contained a comment at the top of the read(Uint8List bytes) method:

 // Utils for reading.

... followed by a bunch inline utility functions. These have all been moved to a separate WavBytesReader class.

Similarly, the Uint8List write() method included the following comment:

// Utils for writing.

Again, all of the subsequent inline utility classes have been moved to a separate WavBytesWriter class.

In order to be able to put those classes in separate files, a couple of other things that were dependencies of both also had to be moved to a separate file (to avoid circular references). So there are some odds and ends in the wav_types.dart and wav_utils.dart files.

Important Note

wav.dart was renamed to wav_file.dart. The reason for this is so that existing code that depended on a single import of wav.dart will continue to function without change. The wav.dart file now simply exports two other files that are required to use this package. This makes it look like a much bigger change than it actually is.

jamescrosswell commented 1 year ago

Hey @liamappelbe... having another crack at it. I tried to keep this as small/tight as possible. It looks much more significant than it is becuase I renamed wav.dart to wav_file.dart.

This pull request is simply moving the reader/writer util functions into their own classes/files... which, I think, makes the Wav.read and Wav.write methods much smaller and easier to reason about.

Let me know your thoughts.

Cheers, James

jamescrosswell commented 1 year ago

I don't see a button to run the tests on my github UI, so I assume that won't be possible until you mark it ready for review.

Yeah it was a draft PR. I've incorporated all your feedback/suggestions and changed the status to "Ready for Review".

Regarding the tests, writeTest('float64', WavFormat.float64) fails the expect on my machine at line 36 (both in this branch and in the main branch):

location [256] is <168> instead of <169>

I imagine all these tests are passing for you, so this might be related to my operating environment.

liamappelbe commented 1 year ago

It looks like CI turned itself off because I haven't had to touch this repo in a while. Maybe that's why I still don't have a button to run the tests on this PR. I'll have to get the bot running again before we can safely submit this PR.

I imagine all these tests are passing for you, so this might be related to my operating environment.

Interesting. What CPU architecture and OS are you using? That test is unnecessarily strict, so I wouldn't be surprised if a tiny difference in the way your CPU handles floats causes it to fail. We can probably just change the function slightly to get it to pass, but that's a separate issue that I wouldn't worry about right now. The important thing is that it passes on the bot.

jamescrosswell commented 1 year ago

Interesting. What CPU architecture and OS are you using?

I'm running on macOS 13.4 (22F66) with an Apple M1.

jamescrosswell commented 1 year ago

Each new file you added needs the copyright header.

Done 👍🏻

liamappelbe commented 1 year ago

Ok, suddenly today I have access to the CI button. Not sure why it wasn't showing up earlier.

liamappelbe commented 1 year ago

Looks like the dart format check is failing, so you'll need to reformat.

The analyis job failure is not related to your change. I'll fix it in a minute.

liamappelbe commented 1 year ago

The rest of the PR is looking good. I've fixed the analysis failure at HEAD, so just git pull it. Once you do that, and fix the formatting issue, I'll approve.