Replace Hpricot with Nokogiri (PR #138)

discourse
#1

I’ve been working on replacing Hpricot with Nokogiri everywhere in the project. It’s used for the oneboxes.

The main reason for this is that we’re using these 2 gems that serve the same purpose, so I think we should choose one of them. As Nokogiri is more actively maintained and has benchmarks that prove it’s faster, I’ve gone with it.

I’ve added specs for the oneboxes that were using Hpricot (amazon, apple, android, flickr and wikipedia), and then I’ve replaced the gems.

The change has been quite transparent, as Nokogiri features an Hpricot compatibility layer.

Before I send a formal pull request, I’d like to hear your opinion on this (maybe there was another reason to use Hpricot), and if everything looks OK, I’ll add another commit to fake the web requests on the specs.

Thanks!

GitHub

#2

:+1:

This is something we’ve wanted to do for a while! The use of hpricot comes to my ignorance of how great Nokogiri is, so after I found out, I had some technical debt to go back and fix the oneboxes. This is great work and definitely should be done.

Let’s throw out hpricot ASAP.

#3

Great, have you got any preference about what to use to fake web requests? Like Fakeweb, VCR, …? El 12/02/2013 17:51, “Robin Ward” notifications@github.com escribió:

[image: :+1:]

This is something we’ve wanted to do for a while! The use of hpricot comes to my ignorance of how great Nokogiri is, so after I found out, I had some technical debt to go back and fix the oneboxes. This is great work and definitely should be done.

Let’s throw out hpricot ASAP.

— Reply to this email directly or view it on GitHubhttps://github.com/discourse/discourse/pull/138#issuecomment-13442309.

#4

No I haven’t done this before so I’d love a suggestion.

#5

Fine, I’ll work on this tomorrow.

#6

awesome +1 would love to see this happen

#7

also … it may make sense to look at extracting onebox into a gem, maybe even simple rack middleware, we would be open to relicensing it under bsd

#8

Extracting onebox into a gem is a great idea, I had thought about this possibility. I’ll certainly contribute to it as well.

#9

OK fine, I’ve added a commit where I set up FakeWeb for the specs. I’ve chosen FakeWeb instead of VCR because I think it’s easier to maintain, and makes the fakes more explicit: you register each faked URI on the specs.

I’m going to document the use of FakeWeb on DEVELOPMENT.md and we’re good to go with the PR if you wish. I’ve seen some other improvements / cleanups for Oneboxer but I think this should be done after the merge.

#10

OK, good to merge when you wish :slight_smile:

#11

Looks like it can’t merge cleanly - is there a conflict with master wow?

(Sorry for all the back and forth!)

#12

There was a conflict on Gemfile.lock, fixed.

#13

Sorry, I should have rebased it instead of merging with master for a cleaner history.

I’m closing this and continuing on https://github.com/discourse/discourse/pull/159

#14