-
Notifications
You must be signed in to change notification settings - Fork 43
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
Adding support for RoXMLElement
#599
Conversation
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.
This starts to look good, thanks for the great work here.
In addition to below comments I have a few more comments/questions
- Is the full
ifXMLEelement
interface going to be implemented here? or that's out of the scope of this PR? - Can you please add some docs to the methods in the
RoXMLElement
class? - Also please add e2e tests such as the following example: callFunc test
callFunc script
super("roXMLElement"); | ||
|
||
this.registerMethods({ | ||
ifXMLEelement: [this.parse, this.getName, this.getNamedElementsCi, this.getAttributes], |
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.
there is a typo in the interface name ifXMLElement
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
}); | ||
} | ||
|
||
public setXMLElem(xmlElem: XmlElement) { |
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.
I think this could be private as it doesn't seem to be used outside this class
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.
Once it's private, this function doesn't need to exist at all! A simple this.xmlNode = xmlElem
would suffice, since we don't seem to be doing anything special here.
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.
ohh, forgot that RoXMLElement
are friend (from C++, where other class/methods can access private members) for other RoXMLElement
objects
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
}, | ||
impl: (interpreter: Interpreter, str: BrsString) => { | ||
try { | ||
this.xmlNode = new XmlDocument(str.value); |
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.
does XmlDocument
return anything to indicate that the string was parsed correctly? something like a boolean value? or does it throws an exception in every 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.
It throws an exception only if the XML can not be parsed at all, but it doesn't if it can parse at least part of XML and it returns true in that case. The similar behavior we have on Roku.
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.
Hey @Vasya-M — thanks for the PR! This will be a very component to have implemented. You're off to a great start, but there's a good bit of cleanup to do before this lands. Most important is a set of end-to-end tests (brightscript files that we execute in jest
test cases) that demonstrate that this works, as @alimnios72 called out. They don't need to be exhaustive tests — just calling each function once or twice to show off the use of roXMLElement
.
I'm looking forward to getting this incorporated!
}); | ||
} | ||
|
||
public setXMLElem(xmlElem: XmlElement) { |
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.
Once it's private, this function doesn't need to exist at all! A simple this.xmlNode = xmlElem
would suffice, since we don't seem to be doing anything special here.
doc for |
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.
Looking much better! Let's just get some proper typing on RoXmlElement#xmlNode
and this should be good to merge 😄
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.
Thanks for the updates @Vasya-M — this seems safe to land assuming everything still works when I remove that test.only
😃
supported interfaces:
see #282