Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ensure that punchblock uses .WAV extension for wav49 format #259

Merged

Conversation

system123
Copy link

Ensures that the uri returned when wav49 format is used, has the same file extension as what asterisk will use.

@benlangfeld
Copy link
Member

Does asterisk never produce .wav49? Is this documented anywhere?

@system123
Copy link
Author

See the related issue: #258

Asterisk hardcodes the same extension replacement.

@coveralls
Copy link

coveralls commented Oct 13, 2016

Coverage Status

Coverage increased (+0.006%) to 99.013% when pulling e318af9 on system123:bugfix/wav49-incorrect-extension into cb30075 on adhearsion:develop.

@bklang
Copy link
Member

bklang commented Oct 13, 2016

I'm pretty sure the correct replacement is WAV, case-sensitive. Did you test this with a real app yet? Also, could we get a spec to cover this behavior?

@system123
Copy link
Author

@bklang you are correct, I'll fix that. It should be .WAV I'll also write a spec.

@system123 system123 force-pushed the bugfix/wav49-incorrect-extension branch from e318af9 to 56582fa Compare October 13, 2016 22:04
@system123
Copy link
Author

Ok changes are now correct, and spec is passing.

@coveralls
Copy link

coveralls commented Oct 13, 2016

Coverage Status

Coverage increased (+0.008%) to 99.014% when pulling 56582fa on system123:bugfix/wav49-incorrect-extension into cb30075 on adhearsion:develop.

@@ -80,7 +80,8 @@ def filename
end

def recording
Punchblock::Component::Record::Recording.new :uri => "file://#{filename}.#{@format}"
uri = "file://#{filename}.#{@format}".gsub('wav49', 'WAV')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One more it: to be safe, the gsub really should only be applied to @format and not the entire filename

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Made the change

@system123 system123 force-pushed the bugfix/wav49-incorrect-extension branch from 56582fa to eaf3298 Compare October 13, 2016 22:17
@coveralls
Copy link

coveralls commented Oct 13, 2016

Coverage Status

Coverage increased (+0.008%) to 99.014% when pulling eaf3298 on system123:bugfix/wav49-incorrect-extension into cb30075 on adhearsion:develop.

Copy link
Member

@benlangfeld benlangfeld left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we get an entry in the changelog also before merging this?

@system123 system123 force-pushed the bugfix/wav49-incorrect-extension branch from eaf3298 to b4a5dcd Compare October 18, 2016 07:08
@coveralls
Copy link

coveralls commented Oct 18, 2016

Coverage Status

Coverage increased (+0.008%) to 99.014% when pulling b4a5dcd on system123:bugfix/wav49-incorrect-extension into cb30075 on adhearsion:develop.

@benlangfeld benlangfeld merged commit 466192a into adhearsion:develop Oct 18, 2016
@benlangfeld
Copy link
Member

Thanks Lloyd. Would you perhaps prep this against Adhearsion 2, which merges Punchblock?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants