Misc features #729
Misc features #729
Conversation
|
Thanks, I'll take a look today. |
| @@ -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 | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
The question is why you would ever want to render an XML node with an
attribute present but empty.
Regards,
Ben Langfeld
Em 13 Jul 2012, às 19:05, Aaron Patterson
[email protected]
escreveu:
@@ -258,7 +259,12 @@ def [] name
###
# Set the attribute value for the attribute +name+ to +value+
def []= name, value
set name.to_s, valueif value.nil?remove_attribute name.to_sIf we wan't this to quack like a hash, shouldn't we implement
deleterather 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.
Reply to this email directly or view it on GitHub:
https://github.com/sparklemotion/nokogiri/pull/729/files#r1160798
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Ok, I'll move this up into a decorator.
|
Objectionable content removed. Any thoughts on the remainder? |
|
Can someone review this please? |
|
I'd really love to get some of this in. |
|
No breaking changes here. @flavorjones @jvshahid merge? |
|
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. |
|
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. |
|
Fine. Please rebase so whitespace changes and revert commits aren't present. |
|
Rebased on master. |
XPath lookup with namespaces with hash keys and XML::Node#[]= should stringify values
|
Merged, thanks. |
No description provided.