Skip to content

Misc features #729

Merged
merged 2 commits into from Oct 5, 2012

3 participants

@benlangfeld

No description provided.

@flavorjones
Sparkle Motion member

Thanks, I'll take a look today.

@tenderlove tenderlove and 1 other commented on an outdated diff Jul 13, 2012
lib/nokogiri/xml/node.rb
@@ -258,7 +259,12 @@ def [] name
###
# Set the attribute value for the attribute +name+ to +value+
def []= name, value
- set name.to_s, value
+ if value.nil?
+ remove_attribute name.to_s
@tenderlove
Sparkle Motion member
tenderlove added a note Jul 13, 2012

If we wan't this to quack like a hash, shouldn't we implement delete rather than having a magic value mean delete? I'm not really excited about the idea of calling a setter that could possibly have the side effect of deleting something.

@tenderlove
Sparkle Motion member
tenderlove added a note Jul 13, 2012

Your scheme definition could demand the presence of the attribute but not define a value. Sorry, I won't accept this feature without a stronger argument.

@benlangfeld
benlangfeld added a note Jul 13, 2012

Ok, I'll move this up into a decorator.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@tenderlove tenderlove and 2 others commented on an outdated diff Jul 13, 2012
lib/nokogiri/xml/node.rb
@@ -258,7 +259,12 @@ def [] name
###
# Set the attribute value for the attribute +name+ to +value+
def []= name, value
- set name.to_s, value
+ if value.nil?
+ remove_attribute name.to_s
+ else
+ value = value.join if value.is_a?(Array)
@tenderlove
Sparkle Motion member
tenderlove added a note Jul 13, 2012

Why? The developer using nokogiri should probably figure out why they're passing arrays and format them the way they want. I really don't like the idea of supporting "laissez-faire" API like this.

@benlangfeld
benlangfeld added a note Jul 13, 2012

Good question. This one snuck in from monkey-patches. Will rebase it out.

@flavorjones
Sparkle Motion member
flavorjones added a note Jul 16, 2012

Worth nothing that "quack like a hash" is on the roadmap for 2.0 (see #666)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@tenderlove tenderlove commented on an outdated diff Jul 13, 2012
test/xml/test_node.rb
@@ -812,6 +832,22 @@ def test_node_equality
assert_equal address1_1, address1_2 # two references to the exact same node
end
+ def test_namespace_search_with_symbol_xpath_and_hash
+ xml = Nokogiri::XML.parse(<<-eoxml)
+ <root>
+ <car xmlns:part="http://general-motors.com/">
+ <part:tire>Michelin Model XGV</part:tire>
+ </car>
+ <bicycle xmlns:part="http://schwinn.com/">
+ <part:tire>I'm a bicycle tire!</part:tire>
+ </bicycle>
+ </root>
+ eoxml
+
+ tires = xml.xpath(:'//bike:tire', {'bike' => 'http://schwinn.com/'})
@tenderlove
Sparkle Motion member
tenderlove added a note Jul 13, 2012

Do people actually write code like this? Since XPath isn't valid for a symbol literal in Ruby, people would actually have to do extra work for this to happen. Supporting "what if" use-cases like this makes me sad.

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

Objectionable content removed. Any thoughts on the remainder?

@benlangfeld

Can someone review this please?

@benlangfeld

I'd really love to get some of this in.

@benlangfeld

No breaking changes here. @flavorjones @jvshahid merge?

@flavorjones
Sparkle Motion member

I don't think the symbol-to-string conversion is useful. There's also a test around non-string attribute values, which doesn't have any corresponding code changes. Can you tell me a little more about that?

In the future, pull requests should not contain reversions, and should not contain multiple features. Following this convention will make it easier to communicate about the pull request and will speed up merging if the PR is accepted.

@benlangfeld

Option 1:

foo.xpath 'ns:bar', 'ns' => 'baz'

Option 2:

foo.xpath 'ns:bar', ns: 'baz'

The latter is better in my view and is what I want my code to look like. You might not like it, but it doesn't do you any harm. Therefore, there is no logical reason not to merge it.

As for the non-string attribute values, there is a change: https://github.com/sparklemotion/nokogiri/pull/729/files#L0R261. Without that change, the test fails.

@flavorjones
Sparkle Motion member

Fine. Please rebase so whitespace changes and revert commits aren't present.

@benlangfeld

Rebased on master.

@flavorjones flavorjones merged commit dae5dcb into sparklemotion:master Oct 5, 2012

1 check failed

Details default The Travis build failed
@flavorjones
Sparkle Motion member

Merged, thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.