-
Notifications
You must be signed in to change notification settings - Fork 521
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
Allow creating generic twiml nodes #357
Conversation
* Parent TwiML object | ||
*/ | ||
/* jshint ignore:end */ | ||
function TwiML() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so ... we could make this an ES6 class
, right? I guess that would require some more significant changes to the generated classes, so if you want to punt then OK. But if we don't do it now, I'm not sure when we would ....
lib/twiml/TwiML.js
Outdated
* comparisons. Save a few CPU cycles. | ||
*/ | ||
/* jshint ignore:end */ | ||
TwiML.prototype._getPropertyName = function _getPropertyName() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
per our convo today, you could make this abstract (uh ... you know what I mean) and have yoyodyne
inject an implementation with the right value into each child class.
lib/twiml/TwiML.js
Outdated
*/ | ||
/* jshint ignore:end */ | ||
TwiML.prototype.toString = function toString() { | ||
return this[this._getPropertyName()].end(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a way to have a non-public getXml
that returns this[this._getPropertyName()]
to avoid duplication?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Duplication of what?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the code this[this._getPropertyName()]
appears in several places -- I thought it would be nice to have a helper method to avoid repeating it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, yes, great point!
lib/twiml/TwiML.js
Outdated
*/ | ||
/* jshint ignore:end */ | ||
TwiML.prototype._getPropertyName = function _getPropertyName() { | ||
return this._propertyName === undefined ? 'response' : this._propertyName; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is it hard to get the response classes to emit a propertyName
as well? this seems like kind of a weird special case
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we can remove this logic now, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
Response
s andTwiML
(added)addText()
method to parentTwiML
classaddChild()
method to anything that inheritsTwiML