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

Corrects to_xml in TwiML generation #318

Merged
merged 2 commits into from
Aug 10, 2017
Merged

Corrects to_xml in TwiML generation #318

merged 2 commits into from
Aug 10, 2017

Conversation

philnash
Copy link
Contributor

Twilio::TwiML::TwiML#to_xml was being aliased to to_s before to_s was defined so took on the old definition.

This PR adds tests for using to_xml and fixes the aliasing. It also removes the reference to builder in the README.

@@ -37,6 +35,7 @@ def to_s(xml_declaration = true)
return ('<?xml version="1.0" encoding="UTF-8"?>' + xml) if xml_declaration
xml
end
alias to_xml to_s
Copy link
Contributor

@ilanbiala ilanbiala Jul 24, 2017

Choose a reason for hiding this comment

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

If the alias has to go in a specific order to work properly, then I think it makes more sense to just explicitly define to_xml and call to_s within the method to avoid confusion.

Also, does this error not exist for:

lib/twilio-ruby/jwt/client_capability.rb
15:      alias to_s to_jwt

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As this is Ruby, it is interpreted and run in order. At the point in the class definition where to_xml is aliased to to_s, to_s had been defined by an ancestor class and to_xml took on that definition. To move it here means that to_xml takes on the correct defintion of to_s.

This didn't cause an issue in client_capability.rb because the ClientCapability class itself doesn't define to_s, it is taken from BaseJWT which was defined before ClientCapability and therefore to_xml takes on the right definition.

There are a couple of articles that describe how alias works, and compare to alias_method:

They both unintentionally show one thing, when using alias Ruby developers do so straight after defining the method they are aliasing to something else. So this placement of alias to_xml to_s is both correct and more rubyish than before.

One other thing, the existence of alias to_s to_jwt in ClientCapability and def to_s ; to_jwt ; end in AccessToken shows that some more refactoring can be done. PR incoming.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah I figured it was interpreted in order. After reading the articles and thinking a bit, I think it makes more sense to explicitly define the method and just call the true function from within, like:

def to_xml
  to_s
end

because there is some controversy. However, are methods defined in order as well? Meaning, if we put the to_xml definition above to_s, will it use some standard to_s method and not one defined later in the class?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There's no controversy, just the right way to use each of the methods, which is as described. Aliasing to_s to to_xml here is perfectly clear.

I think it would look un-Ruby to just do def to_xml ; to_s ; end. It wouldn't be order defined, to_s would be looked up dynamically at runtime.

@codejudas codejudas merged commit f4ea1f8 into master Aug 10, 2017
@childish-sambino childish-sambino deleted the twiml_to_xml branch March 12, 2019 17:09
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.

3 participants