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

[BUGFIX] Fix handling of millisecond values (#48) #49

Merged
merged 1 commit into from
Dec 6, 2017

Conversation

chewi
Copy link
Contributor

@chewi chewi commented Dec 6, 2017

When trying an SSML Break time of 0.5 under Adhearsion, FreeSWITCH
treats this as 0 seconds. This makes some sense as the spec does not
say that you can pass fractional seconds, only whole seconds or
milliseconds with the appropriate suffix.

This change adds the appropriate suffix when rendering XML and
converts back to seconds again when reading XML.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.005%) to 99.093% when pulling e88c85f on bugfix/milliseconds into b8d080c on develop.

2 similar comments
@coveralls
Copy link

Coverage Status

Coverage increased (+0.005%) to 99.093% when pulling e88c85f on bugfix/milliseconds into b8d080c on develop.

@coveralls
Copy link

coveralls commented Dec 6, 2017

Coverage Status

Coverage increased (+0.005%) to 99.093% when pulling e88c85f on bugfix/milliseconds into b8d080c on develop.

Copy link
Member

@sfgeorge sfgeorge left a comment

Choose a reason for hiding this comment

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

Looks great to my eye 👍

Nicely done, thank you James! 👍

else
value.to_f
end
end
Copy link
Member

Choose a reason for hiding this comment

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

👌

def set_time_attribute(key, value)
raise ArgumentError, "You must specify a valid #{key} (positive float value in seconds)" unless value.is_a?(Numeric) && value >= 0
self[key] = "#{value}s"
self[key] = value == value.round ? "#{value.to_i}s" : "#{(value * 1000).to_i}ms"
Copy link
Member

Choose a reason for hiding this comment

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

very nicely done 👌

When trying an SSML Break time of 0.5 under Adhearsion, FreeSWITCH
treats this as 0 seconds. This makes some sense as the spec does not
say that you can pass fractional seconds, only whole seconds or
milliseconds with the appropriate suffix.

This change adds the appropriate suffix when rendering XML and
converts back to seconds again when reading XML.
@coveralls
Copy link

coveralls commented Dec 6, 2017

Coverage Status

Coverage increased (+0.008%) to 99.093% when pulling b319b70 on bugfix/milliseconds into 77ffd0e on develop.

2 similar comments
@coveralls
Copy link

Coverage Status

Coverage increased (+0.008%) to 99.093% when pulling b319b70 on bugfix/milliseconds into 77ffd0e on develop.

@coveralls
Copy link

coveralls commented Dec 6, 2017

Coverage Status

Coverage increased (+0.008%) to 99.093% when pulling b319b70 on bugfix/milliseconds into 77ffd0e on develop.

@chewi chewi merged commit 8a577db into develop Dec 6, 2017
@chewi chewi deleted the bugfix/milliseconds branch December 6, 2017 20:23
@chewi
Copy link
Contributor Author

chewi commented Dec 6, 2017

Guys, any chance we could cut a new release with this fix? We need it in production but we're using JRuby and perhaps I'm being stupid but when installing from git, Bundler always gets the platform wrong. It either doesn't build native extensions at all or tries to build the C versions.

@sfgeorge
Copy link
Member

sfgeorge commented Dec 6, 2017

I'm totally in favor 👍

FYI, if it helps, the only way I've successfully deployed a ruby_speech fork is by forcefully adding lib/ruby_speech/ruby_speech.jar to source control after a rake run. That tends to do it. But I don't see any reason not to cut a new release.

That reminds me (oops) can you add an entry related to this ms fix to the CHANGELOG.md?

@chewi
Copy link
Contributor Author

chewi commented Feb 23, 2018

@benlangfeld, @bklang, the boss is asking where this fix is. I would push a new release myself if I could but I guess only you guys can? Pretty please?

@lpradovera
Copy link
Member

LGTM but I do not have release privileges on this one.

@benlangfeld
Copy link
Member

I’ll push a release when I get back to my computer in a few hours. I’ll also give you both access to do so.

@chewi
Copy link
Contributor Author

chewi commented Feb 23, 2018

Awesome, thanks very much. I'll be putting it into action on Monday.

@benlangfeld
Copy link
Member

benlangfeld commented Feb 24, 2018

https://rubygems.org/gems/ruby_speech/versions/2.4.0
https://rubygems.org/gems/ruby_speech/versions/2.4.0-java

You both have release permission; for punchblock and adhearsion also.

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.

5 participants