-
Notifications
You must be signed in to change notification settings - Fork 176
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
(rebase) Namespace writing - resolves #52 #59
Conversation
spec/xml/namespace_spec.rb
Outdated
output.root = VApp.from_xml(@xml).to_xml | ||
pending "Full namespace write support" | ||
output.should == ROXML::XML.parse_string(@xml) | ||
namespaces = {'vmw' => "http://foo.example.com", 'gronk' => "http://gronk.example.com"} |
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 variable isn't referenced
lib/roxml.rb
Outdated
@@ -42,6 +42,7 @@ def to_xml(params = {}) | |||
ref.update_xml(root, value) | |||
end | |||
end | |||
params[:namespaces].each { |prefix, url| root.add_namespace_definition(prefix, url) } if params[:namespaces] |
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.
Could you shorten this line? It looks on the long side...
Good catches - thanks for the review |
It turns out there was a logical issue in #32 - which caused empty elements without a default to get skipped in the output unless they had a default. I added a failing test of this, confirm that test passed prior to #32, then updated the code added on #32 to make it pass again. Somehow I made a mess of the commit history, I will try to sort that out so it's cleaner to merge |
edcb188
to
4b68962
Compare
Sorry for the noise - I reset my branch, rebased and reapplied the commits and I think this is ready to look at now. |
This rebases #52 to add support for writing namespaces.
Like the original requester I'm open to feedback on this, I just wanted to get that PR updated and ready to either merge, ready for feedback on what else to do, or else reject the whole idea.