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

Add method #wrap to Nokogiri::XML::Element #1531

Closed
wants to merge 2 commits into from
Closed

Add method #wrap to Nokogiri::XML::Element #1531

wants to merge 2 commits into from

Conversation

ethirajsrinivasan
Copy link
Contributor

No description provided.

@ethirajsrinivasan
Copy link
Contributor Author

The build for travis has failed since rvm 2.2.5 in mac OS is not installed .It is an issue as per travis-ci/travis-ci#6467

@flavorjones
Copy link
Member

Hello! Thank you so much for submitting this! I'd love to add a #wrap method.

I have a few suggestions, though, if you don't mind hearing me out.

First, #wrap seems useful for more than just Elements. I'd like to be able to wrap Comment nodes and Text nodes as well. What would you think about moving this method into XML::Node?

I realize that not every node type will be wrappable, but the fact that NodeSet#wrap doesn't care about this today means I really don't care about it for Node, either.

Which brings me to my second suggestion, which is to re-implement NodeSet#wrap to delegate to the contents. Something like this might be appropriate:

def wrap html
  map { |node| node.wrap html }
end

I apologize that I haven't replied before now. If you're not interested in making these changes, please let me know, because I think this is a good feature to have.

@ethirajsrinivasan
Copy link
Contributor Author

@flavorjones I would love to make these changes.Will update my PR.

@ethirajsrinivasan
Copy link
Contributor Author

@flavorjones i have made changes to the PR please have a look at it.

@flavorjones
Copy link
Member

Thanks for making the suggested changes. I'd like to take some time to look at NodeSet#wrap, since it looks like the builder-as-block-argument should be broken; but tests passed. This indicates to me we're missing some test coverage, and I'd like to investigate.

@ethirajsrinivasan
Copy link
Contributor Author

@flavorjones any improvements on the PR

@flavorjones flavorjones added this to the next milestone Nov 8, 2018
flavorjones added a commit that referenced this pull request Dec 2, 2018
and put them in:

- test_node.rb
- test_text.rb

also added test coverage for this in

- test_node_set.rb

this is a follow-on commit to #1531 by @ethirajsrinivasan
@flavorjones flavorjones mentioned this pull request Dec 2, 2018
@flavorjones
Copy link
Member

I'm so sorry for my slow responses. This work needs to be rebased onto master to pass tests; I've done this at #1828. As soon as it goes green in CI, I'll merge it.

@flavorjones
Copy link
Member

Merging manually! Thank you again for your patience and for the great API improvement. Your contribution will be noted in the CHANGELOG.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants