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.

Sign up for GitHub

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

Jump to bottom

First scratch of rexml implementation #46

Merged
merged 2 commits into from
Jan 17, 2017
Merged

First scratch of rexml implementation #46

merged 2 commits into from
Jan 17, 2017

Conversation

fanantoxa
Copy link
Collaborator

@fanantoxa fanantoxa commented Aug 16, 2016 •
edited
Loading

Related to task: Update XmlBuilder to use REXML class

  • Check that works good
  • Check strange parsing. — on xml_builder_spec.rb was printed as — (long dash)
  • Refactor code to be more well designed

@soulcutter Please take a look here.
That is the first scratch but tests works good. Also I've turned on xml output_type for Ox too.

@fanantoxa fanantoxa force-pushed the rexml-implementation branch from 5026169 to 28d40cf Compare August 16, 2016 20:38
@fanantoxa fanantoxa self-assigned this Aug 17, 2016
@fanantoxa fanantoxa mentioned this pull request Aug 18, 2016
Closed
@fanantoxa fanantoxa force-pushed the rexml-implementation branch from 28d40cf to 1c42a1b Compare August 24, 2016 14:03
@fanantoxa
Copy link
Collaborator Author

@soulcutter I've update a bit the code. Please check it)

soulcutter
soulcutter reviewed Aug 24, 2016
View reviewed changes
lib/saxerator/builder/xml_builder.rb
module Saxerator
module Builder
class XmlDocument < REXML::Document
Copy link
Owner

Choose a reason for hiding this comment

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

Why is there a need for a to_xml alias? Less might be more here - meaning we just return a bare REXML::Document and let end-users transform it however they prefer.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@soulcutter Before this we had a Nokogiri document and he has a :to_xml method.
I thought that it'll be good to backward compatibility.
If it's bad Idea I can remove it

Copy link
Owner

Choose a reason for hiding this comment

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

I'm for removing it and simply returning the REXML::Document object. Technically this library isn't 1.0, so this is a tolerable breaking change.

@fanantoxa
Copy link
Collaborator Author

@soulcutter Also question about output. REXML convert &#x2014; to — (long dash). Is it ok or I have to check how to save &#x2014; on output string?

@fanantoxa fanantoxa force-pushed the rexml-implementation branch from 1c42a1b to b63ceb0 Compare August 25, 2016 11:06
@fanantoxa
Copy link
Collaborator Author

@soulcutter I've removed out_put type restrictions for different parsers.

@fanantoxa fanantoxa requested a review from soulcutter December 28, 2016 22:11
soulcutter
soulcutter requested changes Dec 29, 2016
View reviewed changes
Copy link
Owner

@soulcutter soulcutter left a comment

Choose a reason for hiding this comment

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

Everything looks alright other than the wrapper for REXML::Document

README.md
@@ -100,7 +100,7 @@ end
| Setting | Default | Values | Description
|:------------------|:--------|-----------------|------------
| `adapter` | `:nokogiri` | `:nokogiri`, `:ox` | The XML parser used by Saxerator |
| `output_type` | `:hash` | `:hash`, `:xml` | The type of object generated by Saxerator's parsing. `:hash` should be self-explanatory, `:xml` generates a `Nokogiri::XML::Document`
| `output_type` | `:hash` | `:hash`, `:xml` | The type of object generated by Saxerator's parsing. `:hash` should be self-explanatory, `:xml` generates a `REXML::Document` (but have additional method :to_xml)
Copy link
Owner

Choose a reason for hiding this comment

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

Removing our REXML::Document wrapper means we can remove the documentation of to_xml here

spec/lib/builder/xml_builder_spec.rb
</contributor>
</entry>
eos
expected_xml = '<?xml version=\'1.0\'?><entry><id>1</id><published>2012-01-01T16:17:00-06:00</published><updated>2012-01-01T16:17:00-06:00</updated><link href="https://example.com/blog/how-to-eat-an-airplane"/><title>How to eat an airplane</title><content type="html">&lt;p&gt;Airplanes are very large — this can present difficulty in digestion.&lt;/p&gt;</content><media:thumbnail url="http://www.gravatar.com/avatar/a9eb6ba22e482b71b266daadf9c9a080?s=80"/><author><name>Soul&lt;utter</name></author><contributor type="primary"><name>Jane Doe</name></contributor><contributor><name>Leviticus Alabaster</name></contributor></entry>'
expect(entry.to_xml).to eq(expected_xml)
Copy link
Owner

Choose a reason for hiding this comment

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

to_xml will need to be to_s here once the REXML::Document wrapper is removed.

spec/lib/saxerator_spec.rb
let(:output_type) { :xml }
specify { expect(parser.all).to be_a Nokogiri::XML::Document }
specify { expect(parser.all).to be_a Saxerator::Builder::XmlDocument }
Copy link
Owner

Choose a reason for hiding this comment

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

It'll be a REXML::Document when the wrapper is removed

@soulcutter
Copy link
Owner

REXML convert — to — (long dash). Is it ok or I have to check how to save — on output string?

I'm fine with keeping with REXML's behavior for that output.

@fanantoxa fanantoxa force-pushed the rexml-implementation branch 2 times, most recently from 4c9ecc0 to 2bec86d Compare January 16, 2017 14:38
@fanantoxa
Copy link
Collaborator Author

@soulcutter I've updated with your additions, But for some reason test fails on ruby 2.0 with error:

An error occurred while installing nokogiri (1.7.0.1), and Bundler cannot
continue.
Make sure that `gem install nokogiri -v '1.7.0.1'` succeeds before bundling.
The command "eval bundle install --without coverage --binstubs --path=${BUNDLE_PATH:-vendor/bundle}" failed. Retrying, 2 of 3.

@fanantoxa
Copy link
Collaborator Author

@soulcutter Also look like test doesn't works properly because the not test when we changing adapter actually. It's stubbed.
So when I've tried to use it on app if failed because wasn't able to find adapter to require.

@fanantoxa fanantoxa force-pushed the rexml-implementation branch from 976c44a to 2bec86d Compare January 16, 2017 16:19
@fanantoxa fanantoxa force-pushed the rexml-implementation branch from 2bec86d to c865cf9 Compare January 16, 2017 16:19
@soulcutter
Copy link
Owner

The latest nokogiri requires ruby 2.1.0+

@soulcutter
Copy link
Owner

So when I've tried to use it on app if failed because wasn't able to find adapter to require.

It should fail in that scenario. You have to have the adapter on the load path if you want to use it. That could be worth clarifying in the documentation. Adapters are external dependencies, not bundled with this library.

@soulcutter soulcutter merged commit e5c32bd into master Jan 17, 2017
@soulcutter soulcutter deleted the rexml-implementation branch January 17, 2017 02:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@soulcutter soulcutter soulcutter approved these changes

Assignees

@fanantoxa fanantoxa

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@fanantoxa @soulcutter