Skip to content
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

Added missing XML::Attributes#delete #6910

Merged
merged 1 commit into from
Oct 6, 2018

Conversation

joenas
Copy link
Contributor

@joenas joenas commented Oct 4, 2018

Hello!

This adds the missing XML::Attributes#delete referenced in #3902. I wasn't sure about what kind of return value that would be appropriate so I went with what felt natural.

@straight-shoota
Copy link
Member

"Delete" methods usually return the value that is being deleted. I think this would make sense here, too. nil would indicate that the attribute doesn't exist.

@joenas joenas force-pushed the add_xml_attributes_delete branch from 17e15c8 to 2f16a68 Compare October 4, 2018 17:21
@joenas
Copy link
Contributor Author

joenas commented Oct 4, 2018

Done, @straight-shoota!

res = root.delete("bar")
root["bar"]?.should be_nil
root.to_s.should eq(%{<foo/>})
res.should eq "bar"
Copy link
Member

Choose a reason for hiding this comment

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

I meant it to return the value, in this case baz.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right

@joenas joenas force-pushed the add_xml_attributes_delete branch from 2f16a68 to b47e068 Compare October 4, 2018 17:48
Fixes from PR feedback

More pr fixes

Be gone
@joenas joenas force-pushed the add_xml_attributes_delete branch from b47e068 to aaef611 Compare October 4, 2018 17:49
@joenas
Copy link
Contributor Author

joenas commented Oct 4, 2018

🎊 🎈

Copy link
Member

@sdogruyol sdogruyol left a comment

Choose a reason for hiding this comment

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

Thank you @joenas 👍

@sdogruyol sdogruyol merged commit 4fb09af into crystal-lang:master Oct 6, 2018
@sdogruyol sdogruyol added this to the 0.27.0 milestone Oct 6, 2018
@joenas
Copy link
Contributor Author

joenas commented Oct 6, 2018

Happy to help @sdogruyol 😊

@joenas joenas deleted the add_xml_attributes_delete branch October 30, 2018 18:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants