allow setting and deleting of attributes on supported XML::Node types by bmmcginty · Pull Request #3902 · crystal-lang/crystal

bmmcginty · 2017-01-16T05:38:45Z

Allow node[name]=value and node.attributes.delete(name)

I'm not sure if Attributes#delete is the correct place for the delete method, but I'm open to suggestions. Maybe Node#delete_attribute?
Also, are we avoiding all methods in libxml2 that require the LIBXML_TREE_ENABLED define? (I've continued that pattern in this PR.)
That should probably be noted somewhere if so.

asterite · 2017-01-16T12:54:33Z

Thank you for this!

I initially wrote the libxml2 bindings and I mainly focused on parsing XML, not so much in modifying it afterwards. So that the code is not using functions that depend on LIBXML_TREE_ENABLED is just a coincidence. I would guess that flag is true by default in most systems, so don't hesitate to use any kind of function from libxml2.

Some comments:

What do you think if we make node["attr"] = nil remove the attribute? It seems reasonable to me, and that way one can pass a value that could be String or Nil and have the attribute set or removed based on that.
I wouldn't raise an exception when trying to delete an attribute that doesn't exist, just like deleting a key from a Hash, when the key doesn't exist, doesn't raise

asterite · 2017-01-16T16:17:22Z

src/xml/attributes.cr

+  def []=(name : String, value : String)
+    curr = self[name]?
+    if curr
+      curr.not_nil!.content = value


This not_nil! is not needed because you already checked that it's not nil with if curr above

bmmcginty · 2017-01-16T18:49:02Z

No more raising and Node[attr]=nil now deletes.
f

asterite · 2017-01-20T16:02:47Z

Hi @bmmcginty ,

Sorry for what I'm going to say, but we had a discussion about this with @bcardiff and @spalladino and we finally decided that:

node[name] = value could look really weird if value might be nil and the attribute is deleted, because that looks like an assignment (for example doing that in a Hash will always insert that key/value pair)
node.delete(name) and attributes.delete(name) is better than the above
restrict the []= method to only accept String as value (like the code you proposed, but without the Nil overload I proposed)

In Nokogiri you can assign any type as the value, for example 1 or nil, and it ends up converting that value to string and assigning it. But it's a bit weird to do node[name] = nil and then having node[name] be the empty string, so we are not sure about copying that behaviour. If you want you can open an issue to discuss that, but it's low priority.

In short... could you revert your commits to your original proposal? 😊

Thank you!

bmmcginty · 2017-01-23T20:55:45Z

No problem. That completely makes sense.

bmmcginty · 2017-01-23T21:26:49Z

Fixed. []=(string,nil) removed. node.delete calls attributes.delete. attributes.delete deletes attribute if exists, and returns the nodes content or nil if non-existing attribute.

Sija · 2017-01-31T12:57:05Z

src/xml/node.cr

    ptr ? (ptr.as(Array(XML::Error))) : nil
  end
-end
+end


No newline at the end of file.

bmmcginty · 2017-02-01T10:37:53Z

@Sija added blank line. travis builds seem to be timing out, though I don't have a Mac to test on.

RX14 · 2017-02-01T16:08:43Z

@bmmcginty the linux builds are failing due to a formatter error. I think the mac builds died due to a travis incident.

Sija · 2017-02-01T16:23:20Z

@bmmcginty I'd suggest running crystal format to have it all fixed in one go.

bmmcginty · 2017-02-05T04:11:30Z

@Sija Format has been run and code rebased. Error appears to be just that odd mac one on Travis. Thanks for your patience.

RX14 · 2017-02-05T12:37:40Z

@bmmcginty CI on OSX has been fixed on master, if you rebase your PR onto master it should be fixed.

bmmcginty · 2017-02-08T05:23:32Z

@RX14 rebased, pushed, and passed. thanks.

Sija · 2017-03-12T16:40:28Z

src/xml/node.cr

    attributes[attribute]?.try &.content
  end

+  # sets *attribute* of this node to *value*. Raises `XML::Error` if this node does not support attributes.


sets -> Sets

Double newline before 2nd sentence would do good.

Sija · 2017-03-12T16:41:16Z

src/xml/node.cr

  end

  # Gets the attribute content for the *attribute* given by name.
-  #


what's the reason for removal?

Sija · 2017-03-12T16:41:22Z

src/xml/node.cr

  end

  # Gets the attribute content for the *attribute* given by name.
-  #


Sija · 2017-03-12T16:42:33Z

src/xml/node.cr

+  end
+
+  # Deletes attribute given by *name*.
+  # Returns attributes value, or `nil` if attribute not found.


Extra newline in between would do good. See previous comments.

@Sija there is no consistency regarding if the docs for the returned / raised should be in a new paragraph. Applying your proposed changes will left xml/node docs inconsistent.

Let's merge this as it is and unify formatting later. I agree a new line looks better in the html rendering but in an .cr file I would probably forgot to include it.

bcardiff · 2017-03-13T18:10:35Z

Thanks @bmmcginty

joenas · 2018-10-03T15:58:01Z

Is it just me or has there never been a XML::Attributes#delete?

require "xml"
xml = XML.parse("<div></div>")
xml.delete("test")
=> undefined method 'delete' for XML::Attributes

RX14 · 2018-10-03T16:13:10Z

@joenas looks like a bug, can you report it?

asterite · 2018-10-03T16:17:38Z

There's unlink

joenas · 2018-10-03T16:38:12Z

@RX14 absolutely, issue here or elsewhere?

bcardiff · 2018-10-03T17:24:13Z

@joenas either open a new issue or, if you have time, a PR fixing it with specs. Thanks!

joenas · 2018-10-03T17:34:06Z

I’ll see what I can do :) spending most of my free time porting readability to crystal atm

bmmcginty · 2018-10-03T17:52:52Z

If you haven't done this yet, let me know and I'll do it this afternoon. (I might be responsible for this myself.)

…

On Wed, Oct 03, 2018 at 05:34:37PM +0000, Jon Neverland wrote: Iâ��ll see what I can do :) spending most of my free time porting readability to crystal atm — You are receiving this because you were mentioned. Reply to this email directly, [view it on GitHub](http://go.bmcginty.us/274708) , or [mute the thread](http://go.bmcginty.us/274709) .![]

joenas · 2018-10-04T06:08:37Z

@bmmcginty Sorry I just saw this, I'm i CET so... anyways I haven't so feel free!

joenas · 2018-10-04T14:18:05Z

@bmmcginty Actually I need this to get my readability port working and it looks like I managed to figure it out... so I could put up a PR if you like

bmmcginty · 2018-10-04T15:39:47Z

Please do. Thanks.

joenas · 2018-10-04T16:09:16Z

Done: #6910

bcardiff added kind:feature pr:wip status:accepted topic:stdlib labels Jan 16, 2017

asterite reviewed Jan 16, 2017

View reviewed changes

bmmcginty force-pushed the xml_set_delete_attribute branch from a12491f to c8a96b7 Compare January 23, 2017 21:24

bmmcginty force-pushed the xml_set_delete_attribute branch from c8a96b7 to 54faf04 Compare January 27, 2017 03:44

mverzilli added pr:needs-review and removed pr:wip labels Jan 27, 2017

Sija reviewed Jan 31, 2017

View reviewed changes

bmmcginty force-pushed the xml_set_delete_attribute branch from acdae9c to 85bef20 Compare February 2, 2017 07:04

allow setting and deleting of attributes on supported XML::Node types

168621a

bmmcginty force-pushed the xml_set_delete_attribute branch from 85bef20 to 168621a Compare February 7, 2017 13:59

Sija reviewed Mar 12, 2017

View reviewed changes

src/xml/node.cr

end

# Gets the attribute content for the *attribute* given by name.

#

Copy link

Contributor

Sija Mar 12, 2017

Choose a reason for hiding this comment

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

what's the reason for removal?

Sija reviewed Mar 12, 2017

View reviewed changes

src/xml/node.cr

end

# Gets the attribute content for the *attribute* given by name.

#

Copy link

Contributor

Sija Mar 12, 2017

Choose a reason for hiding this comment

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

ditto

Sija reviewed Mar 12, 2017

View reviewed changes

Apply comment formmating suggestion

e5a9945

bcardiff force-pushed the xml_set_delete_attribute branch from 7e90aec to e5a9945 Compare March 13, 2017 17:01

bcardiff merged commit f1a1adb into crystal-lang:master Mar 13, 2017

bcardiff removed the pr:needs-review label Mar 13, 2017

bcardiff added this to the Next milestone Mar 13, 2017

joenas mentioned this pull request Oct 4, 2018

Added missing XML::Attributes#delete #6910

Merged

Uh oh!

Conversation

bmmcginty commented Jan 16, 2017

Uh oh!

asterite commented Jan 16, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bmmcginty commented Jan 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

asterite commented Jan 20, 2017

Uh oh!

bmmcginty commented Jan 23, 2017

Uh oh!

bmmcginty commented Jan 23, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bmmcginty commented Feb 1, 2017

Uh oh!

RX14 commented Feb 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Sija commented Feb 1, 2017

Uh oh!

bmmcginty commented Feb 5, 2017

Uh oh!

RX14 commented Feb 5, 2017

Uh oh!

bmmcginty commented Feb 8, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bcardiff commented Mar 13, 2017

Uh oh!

joenas commented Oct 3, 2018

Uh oh!

RX14 commented Oct 3, 2018

Uh oh!

asterite commented Oct 3, 2018

Uh oh!

joenas commented Oct 3, 2018

Uh oh!

bcardiff commented Oct 3, 2018

Uh oh!

joenas commented Oct 3, 2018

Uh oh!

bmmcginty commented Oct 3, 2018 via email

Uh oh!

joenas commented Oct 4, 2018

Uh oh!

joenas commented Oct 4, 2018

Uh oh!

bmmcginty commented Oct 4, 2018 via email

Uh oh!

joenas commented Oct 4, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

bmmcginty commented Jan 16, 2017 •

edited

Loading

RX14 commented Feb 1, 2017 •

edited

Loading