Add parameter so text can be optionally not escaped for tag!

[davispuh]

If I want to add node such as apples & oranges without double escaping it, I've to do it with block. such as

xml = Builder::XmlMarkup.new({:indent => 2})
xml.root {
    xml.node { xml << 'apples &amp; oranges' }
}

but output isn't nice...

<root>
  <node>
apples &amp; oranges  </node>
</root>

_indent is private and so it wouldn't be easy to add indentation myself.

With this PR, I added option to tag! that you can specify whether to escape text or not. So we can simply

xml.node(false, 'apples & oranges')

we get nice result

<root>
  <node>apples &amp; oranges</node>
</root>

with current implementation, or with xml.node(true, 'apples & oranges') or xml.node('apples & oranges') it would be double escaped

<root>
  <node>apples &amp;amp; oranges</node>
</root>

[jimweirich]

I don't particularly like the bare true/false as the first argument. It's not immediately clear what xml.node(true, "XX") means.

I think I would like xml.node("XX", indent: false) better, but I haven't checked to see if that syntax interferes with anything else.

Thoughts?

[tjarratt]

+1 for this PR.

Context: I'd like to merge in a PR that's dependent on these changes: savonrb/gyoku#35 (comment)

@jimweirich, instead of a boolean, how about the symbol? I suggest :raw or maybe dont_escape.

My personal choice (and I believe the most idiomatic) would be to pass optional values like this in a hash eg: {:dont_escape_text => true} but since #tag! already takes var args with a hash, I'm unsure how you'd be able to do this.

[davispuh]

oops, sorry I somehow missed/forgot about this PR.

Anyway my implementation is such because there I couldn't think of any other way how to make this possible. Only way to implement it differently would be with dropping some feature as current way is very flexible for creating tag, but not how to specify options.

Also that true and false doesn't matter which arg it is, it will work in any order, like xml.node("text", false)

I'll explain why currently it's not possible to make it different.

• Firstly, you can't use Hash as it's already used for attributes xml.node({:a=>'b'}) will create <node a="b"/>
• You can't reuse hash, because someone might need attribute named like that
• You can't implement it based on argument position, because in current implementation order doesn't matter. xml.node('c', {:a=>'b'}) is same as xml.node({:a=>'b'}, 'c')
• So there's no limit for arguments, order doesn't matter and xml.node('tr', 'olo', 'lol') will create <node>trololol</node>

Actually there are few ways. One way would be using Symbol indeed (but it can't be first arg as it's used for xml namespace) so xml.node('text', :raw), another option would be using array which contains hash, eg. xml.node('text', [{escape: false}]) and I guess last way would be using symbol as hash value (eg. xml.node('text', {:escape => :no})), basically if hash's value is a symbol and not a string we assume it's option, but this still might interfere if someone used to generate such xml and current behavior would convert that symbol to string.

[kristianmandrup]

I think this is a bit of a "whacky" discussion. I think the main "problem" is the decision to stick with a single method xml and try to fit all configuration on the method alone.
A more flexible approach would be to allow for chaining, where each method chained is meant for a specific kind of configuration. Another option might be to employ a "block" config approach:

Gyoky.config do |config|
  config.xml ...
  config.node ...
  config.print ...
end

# or
Gyoky.xml(...).print(...).config(...)

Much simpler and easier to extend, understand etc.

[davispuh]

It's Builder::XmlBase class which makes all XML markup, it's not related to anything else. I guess could create subclass which would allow some different options so this still would be same backwards compatible.

Adding global option for escaping isn't that useful, because almost always you want to escape markup and only few times you need to directly add raw markup. Thus you need that option per tag, not global.

And method chaining isn't really related to this issue at all, it's that current XmlBase implementation just doesn't allow much ways for configuration. What to do, it's all up to @jimweirich . Need to choose how to implement this. I don't really care if it's this way or you can remake it differently.

[tjarratt]

👍 for @kristianmandrup's suggestions. Adding a symbol to the args passed to #node is a good solution that doesn't require breaking the existing API, however.