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

Caching support for preview #97

Merged
merged 48 commits into from
Sep 10, 2013
Merged

Caching support for preview #97

merged 48 commits into from
Sep 10, 2013

Conversation

vykster
Copy link
Contributor

@vykster vykster commented Aug 22, 2013

No description provided.

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling 7fe4d63 on caching-support-for-preview into 06830a5 on master.

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling 579fe84 on caching-support-for-preview into 06830a5 on master.

@vykster
Copy link
Contributor Author

vykster commented Aug 27, 2013

Exploring https://github.com/minad/moneta for setting cache expire times.

@@ -11,7 +12,7 @@
require_relative "onebox/engine"

module Onebox
def self.preview(url, args={})
Preview.new(url, args)
def self.preview(url, options = { cache: Hash.new })

Choose a reason for hiding this comment

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

We should move this default somewhere, like a singleton method. Something like:

def self.defaults
  {
    cache: Hash.new
  }
end

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling fa79403 on caching-support-for-preview into 06830a5 on master.

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling f269840 on caching-support-for-preview into 06830a5 on master.

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling a122c3b on caching-support-for-preview into 06830a5 on master.

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling 07ca015 on caching-support-for-preview into e9e29bc on master.

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling c7f7a69 on caching-support-for-preview into e9e29bc on master.

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling c7f7a69 on caching-support-for-preview into e9e29bc on master.

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling 80b556e on caching-support-for-preview into e9e29bc on master.

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling 80b556e on caching-support-for-preview into e9e29bc on master.

end

def raw
@raw ||= Nokogiri::HTML(open(@url))

Choose a reason for hiding this comment

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

Lets move this into it's the HTML module, just like the Onebox module. That means we'll need to change this to

def raw
  raise NoMethodError, "your engine needs to implement this method"
end

and then write a test that makes sure this method doesn't raise an error, just like data.

You'll also need to include HTML into all the oneboxes that just use nokogiri.

Choose a reason for hiding this comment

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

Also write some documentation on what this method needs to do on individual types of engines.

Copy link
Contributor

Choose a reason for hiding this comment

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

Should we test HTML in engine_spec.rb then? Or have a HTML spec? We don't have one currently for OpenGraph...

Choose a reason for hiding this comment

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

Yes, excellent idea.

And we should.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok

vykster and others added 24 commits September 9, 2013 21:01
@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling 0e515a8 on caching-support-for-preview into fa61d5d on master.

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling 0e515a8 on caching-support-for-preview into fa61d5d on master.

jzeta added a commit that referenced this pull request Sep 10, 2013
@jzeta jzeta merged commit 3f20142 into master Sep 10, 2013
@jzeta jzeta deleted the caching-support-for-preview branch September 10, 2013 04:11
@vykster
Copy link
Contributor Author

vykster commented Sep 10, 2013

Yeah!

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

Successfully merging this pull request may close these issues.

None yet

4 participants