Feature req: adapters for ruby's xmlrpc class #84

Closed
rb2k opened this Issue Nov 7, 2013 · 14 comments

Comments

Projects
None yet
3 participants
@rb2k

rb2k commented Nov 7, 2013

It would be neat to be able to use ox to provide XMLRPC parsing in https://github.com/ruby/ruby/blob/trunk/lib/xmlrpc/parser.rb

I think the things to look at would be the AbstractStreamParser and AbstractTreeParser classes

@ohler55

This comment has been minimized.

Show comment Hide comment
@ohler55

ohler55 Nov 7, 2013

Owner

I'm not exactly sure what you are looking for but with the Ox callback parser (SAX) you should be able to drive your XMLRPC calls. You will have to either rename some of them or wrap them but I don't see a problem with doing so. If I am missing the point please explain in more details what you are looking for.

Owner

ohler55 commented Nov 7, 2013

I'm not exactly sure what you are looking for but with the Ox callback parser (SAX) you should be able to drive your XMLRPC calls. You will have to either rename some of them or wrap them but I don't see a problem with doing so. If I am missing the point please explain in more details what you are looking for.

@rb2k

This comment has been minimized.

Show comment Hide comment
@rb2k

rb2k Nov 7, 2013

I thought it would be nice to have the matching adapter code in the ox gem that integrates within the stdlib xmlrpc client. Basically that code that "wraps them" so people don't have to write it themselves.

Examples that are included in stdlib:

REXML Stream parser: https://github.com/ruby/ruby/blob/trunk/lib/xmlrpc/parser.rb#L723-L746
NqXML Stream parser: https://github.com/ruby/ruby/blob/trunk/lib/xmlrpc/parser.rb#L614-L618

You can use .set_parser() on the XMLRPC client and pass in a class
So I'd love to see ox ship with the sax/regular parser classes so people could do something like:

require 'rubygems'
require 'xmlrpc/client'
require 'ox/xmlrpc_adapters'

bla = XMLRPC::Client.new3(...)
bla.set_parser(Ox::XMLRPC_ADAPTERS::Streamparser)

Seeing as XMLRPC is part of the stdlib, integrating in that doesn't seem tooooo far fetched :)

rb2k commented Nov 7, 2013

I thought it would be nice to have the matching adapter code in the ox gem that integrates within the stdlib xmlrpc client. Basically that code that "wraps them" so people don't have to write it themselves.

Examples that are included in stdlib:

REXML Stream parser: https://github.com/ruby/ruby/blob/trunk/lib/xmlrpc/parser.rb#L723-L746
NqXML Stream parser: https://github.com/ruby/ruby/blob/trunk/lib/xmlrpc/parser.rb#L614-L618

You can use .set_parser() on the XMLRPC client and pass in a class
So I'd love to see ox ship with the sax/regular parser classes so people could do something like:

require 'rubygems'
require 'xmlrpc/client'
require 'ox/xmlrpc_adapters'

bla = XMLRPC::Client.new3(...)
bla.set_parser(Ox::XMLRPC_ADAPTERS::Streamparser)

Seeing as XMLRPC is part of the stdlib, integrating in that doesn't seem tooooo far fetched :)

@ohler55

This comment has been minimized.

Show comment Hide comment
@ohler55

ohler55 Nov 7, 2013

Owner

got it. Good idea. I'll start exploring a little. No promises though.

Owner

ohler55 commented Nov 7, 2013

got it. Good idea. I'll start exploring a little. No promises though.

@rb2k

This comment has been minimized.

Show comment Hide comment
@rb2k

rb2k Nov 7, 2013

I looked into it this morning, but had to switch over to another task after a bit.
It seems doable, so far my only problem was the way you have to subclass the ox sax parser handler but then call Ox.sax_parse. Should be doable though.

rb2k commented Nov 7, 2013

I looked into it this morning, but had to switch over to another task after a bit.
It seems doable, so far my only problem was the way you have to subclass the ox sax parser handler but then call Ox.sax_parse. Should be doable though.

@ohler55

This comment has been minimized.

Show comment Hide comment
@ohler55

ohler55 Nov 7, 2013

Owner

Certainly possible. Easy enough to call sax_parse from the subclass too. If you are interested in working together on it we can take it offline to email and start a branch.

Owner

ohler55 commented Nov 7, 2013

Certainly possible. Easy enough to call sax_parse from the subclass too. If you are interested in working together on it we can take it offline to email and start a branch.

@ohler55

This comment has been minimized.

Show comment Hide comment
@ohler55

ohler55 Dec 1, 2013

Owner

Any further interest or should I close the issue for now?

Owner

ohler55 commented Dec 1, 2013

Any further interest or should I close the issue for now?

@rb2k

This comment has been minimized.

Show comment Hide comment
@rb2k

rb2k Dec 2, 2013

I just moved to the US, so the last week was a bit busy :)
I am however still interested and hope to work on it some time next week.

rb2k commented Dec 2, 2013

I just moved to the US, so the last week was a bit busy :)
I am however still interested and hope to work on it some time next week.

@ohler55

This comment has been minimized.

Show comment Hide comment
@ohler55

ohler55 Dec 2, 2013

Owner

Understand. Same when I moved back from Tokyo at the beginning of the year.

Owner

ohler55 commented Dec 2, 2013

Understand. Same when I moved back from Tokyo at the beginning of the year.

@ohler55

This comment has been minimized.

Show comment Hide comment
@ohler55

ohler55 Jan 15, 2014

Owner

Nice and simple. I like it. Can you move it to the contrib directory though. It doesn't belong in the Ox directly as it can not be pulled in with the oj require since it has external dependencies and it is kind of odd requiring something from inside the module when it has already been loaded.

Owner

ohler55 commented Jan 15, 2014

Nice and simple. I like it. Can you move it to the contrib directory though. It doesn't belong in the Ox directly as it can not be pulled in with the oj require since it has external dependencies and it is kind of odd requiring something from inside the module when it has already been loaded.

@jfontan

This comment has been minimized.

Show comment Hide comment
@jfontan

jfontan Jan 15, 2014

Contributor

The problem moving this file to the contrib directory is that it is not added to the gem so this file will need to be imported manually to the project that uses it.

Another problem I've seen is that Ox does not unescape html/xml, that is, convert < to <, etc. This can be easily fixed changing the "character" alias to a method that calls, for example, CGI.unescape_html. Still most of the libraries to unescape html are awfully slow and most the time won with Ox parsing is lost unescaping the strings.

I'll move the file to contrib if you still think it's the way to go.

Contributor

jfontan commented Jan 15, 2014

The problem moving this file to the contrib directory is that it is not added to the gem so this file will need to be imported manually to the project that uses it.

Another problem I've seen is that Ox does not unescape html/xml, that is, convert < to <, etc. This can be easily fixed changing the "character" alias to a method that calls, for example, CGI.unescape_html. Still most of the libraries to unescape html are awfully slow and most the time won with Ox parsing is lost unescaping the strings.

I'll move the file to contrib if you still think it's the way to go.

@ohler55

This comment has been minimized.

Show comment Hide comment
@ohler55

ohler55 Jan 15, 2014

Owner

It would be a pity to have to make another gem just for the adapter.

Owner

ohler55 commented Jan 15, 2014

It would be a pity to have to make another gem just for the adapter.

@ohler55

This comment has been minimized.

Show comment Hide comment
@ohler55

ohler55 Jan 15, 2014

Owner

It will be in the next release. Thanks. Ox should be doing the unescaping. Can you provide me a short XML where it fails and open an issue for it?

Owner

ohler55 commented Jan 15, 2014

It will be in the next release. Thanks. Ox should be doing the unescaping. Can you provide me a short XML where it fails and open an issue for it?

@ohler55

This comment has been minimized.

Show comment Hide comment
@ohler55

ohler55 Feb 2, 2014

Owner

Finally made the release. Ox 2.1.0

Owner

ohler55 commented Feb 2, 2014

Finally made the release. Ox 2.1.0

@ohler55

This comment has been minimized.

Show comment Hide comment
@ohler55

ohler55 Feb 11, 2014

Owner

Feature is in. Closing.

Owner

ohler55 commented Feb 11, 2014

Feature is in. Closing.

@ohler55 ohler55 closed this Feb 11, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment