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

build xml with ox #1

Merged
merged 13 commits into from Apr 27, 2013
Merged

build xml with ox #1

merged 13 commits into from Apr 27, 2013

Conversation

notezen
Copy link
Contributor

@notezen notezen commented Apr 3, 2013

May be my changes will be interesting for you:

i moved library dependent code that builds xml data into one place,
changed syntax for building xml to library independent and used fast xml library ox

I marked with "TODO xml builder" places where i did not change old code.

@crapooze
Copy link
Owner

Thanks! This is very interesting.
I don't have prior experience with ox hence I'll have a look to it.
Also, I'm very interested in the "Component" login, thanks for patching it out.
Give me some more days :).

@crapooze
Copy link
Owner

Ok, I've browsed through it quickly. You've done interesting work.
Before accepting the merge I'd like to know two things:

  1. Do you think there's an easy way to get a "builder" interface close to nokogiri's xml_builder? my point is that when there are deeply nested nodes (e.g., file-transfer negotiations), the block-passing style of nokogiri's xml_builder allows to capture the XML structure. Something I'm not ready to lose is the ability(see https://github.com/crapooze/em-xmpp/blob/master/lib/em-xmpp/helpers.rb#L200-L220 )

  2. Do you know if there will be some support or API-compatible interface to JRuby with ox? This point is less important that point (1) but I think we should not move too far from being able to port em-xmpp to JRuby

Ideally, we would have an abstraction that can then enable someone to pick between ox and nokogiri interchangeably. What's your take on that?

@notezen
Copy link
Contributor Author

notezen commented Apr 27, 2013

  1. As you can see in https://github.com/notezen/em-xmpp/blob/ox/lib/em-xmpp/helpers.rb#L198-L218
    porting to new interface is very easy. Deeply nested nodes are done with recursive calls to x and x_if methods.
    Also see https://github.com/ohler55/ox/blob/master/contrib/builder_test.rb as an example of usage.
    The reason why I against nokogiri's style is use of "method_missing" . It is relatively slow and I think can not be used in fast environments.
  2. According to homepage https://github.com/ohler55/ox Ox is compatible with Ruby 1.8.7, 1.9.2, JRuby, and RBX.
    You can ask author about details.
  3. I thought about it, it is rational and can be done easy. https://github.com/notezen/em-xmpp/blob/ox/lib/em-xmpp/xml_builder.rb (x and x_if methods)
    and https://github.com/notezen/em-xmpp/blob/ox/lib/em-xmpp/xml_parser.rb are points of change for different xml libraries (not only Nokogiri and Ox)

Author of Ox have planed to write SAX push parser (ohler55/ox#50). After that Ox support in em-xmpp will be complete.

@crapooze
Copy link
Owner

Really good work!
I agree that the method_missing approach is not great (i even warn in the README).
I'm merging it, let's say that the next effort will be to port all entities to your xml_builder approach (plus bin/xmig, which is my debugging swiss-knife for now).

crapooze added a commit that referenced this pull request Apr 27, 2013
build xml with ox
there are some TODO left to update the XML building parts
@crapooze crapooze merged commit 9d52b49 into crapooze:master Apr 27, 2013
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.

None yet

2 participants