search

maddyblue / go-dsp

Digital Signal Processing for Go
http://godoc.org/github.com/mjibson/go-dsp
ISC License
858 stars 89 forks source link

Add unit tests to wav package and refactor #5

Closed nlefler closed 10 years ago

nlefler commented 10 years ago

Moved constants out, some refactoring of sample extraction. Added unit tests for reading wavs.

maddyblue commented 10 years ago

This PR is not mergeable. Could you fix that?

I'm unclear why this refactor is better than the current version. It is 70 lines longer, provides no obvious benefit, and does not make the code more clear. Could you provide arguments for its inclusion?

nlefler commented 10 years ago

I'm unclear why this refactor is better than the current version. It is 70 lines longer, provides no obvious benefit, and does not make the code more clear. Could you provide arguments for its inclusion?

It moves byte offsets into named constants for better readability and testing. It breaks the Wav struct into separate header and data structs which will allow for representing the wav data in different manners later (e.g. the streaming wav change). Moving the header validation, header initialization, and sample reading code into functions makes the code easier to read, allows for extension later, and makes testing possible.

nlefler commented 10 years ago

This PR is not mergeable. Could you fix that?

I don't see where GitHub is saying it's unmergable. Is that true for the latest diff?

maddyblue commented 10 years ago

It says it right above where I'm typing this message. It's still unmergeable.

2014-01-21-151537_847x533_scrot

nlefler commented 10 years ago

Merged commits from master. Hopefully it'll auto-merge now. However I'm not happy about the commit history at this point. Would you rather I rebased?

maddyblue commented 10 years ago

Up to you, but I'd be ok with a rebase.

nlefler commented 10 years ago

Fixed up commits