release v0.0.1 #2
Conversation
| thumbnail.respond_to?(:file) ? thumbnail.file.url(:medium) : thumbnail | ||
| end | ||
|
|
||
| def media_object |
There was a problem hiding this comment.
This does not belong in this Engine. It should not make assumptions about classes defined elsewhere. In the case of europeana-portal-collections, ::MediaObject implement #hash_source_url but it is conceivable that another Rails app using this Engine would have a ::MediaObject model that did not.
We need to move the #media_object and #thumbnail_url logic out of here and back into europeana-portal-collections, refactored there.
| end | ||
| end | ||
|
|
||
| def media_object_url |
| @@ -1,14 +1,16 @@ | |||
| # frozen_string_literal: true | |||
There was a problem hiding this comment.
bundle exec bin/console does not work.
europeana-feed-jobs.gemspec
Outdated
| spec.homepage = 'https://github.org/europeana/europeana-feed-jobs' | ||
| spec.license = 'EUPL-1.1' | ||
|
|
||
| spec.files = Dir['{config,lib}/**/*', '.rubocop.yml', 'Gemfile', 'LICENSE.md', 'README.md'] |
There was a problem hiding this comment.
- There is no config directory.
- There are app and bin directories which should be included in a built gem.
europeana-feed-jobs.gemspec
Outdated
|
|
||
| spec.files = Dir['{config,lib}/**/*', '.rubocop.yml', 'Gemfile', 'LICENSE.md', 'README.md'] | ||
|
|
||
| spec.bindir = 'exe' |
europeana-feed-jobs.gemspec
Outdated
| spec.require_paths = ['lib'] | ||
|
|
||
| spec.add_dependency 'rails', '~> 4.2.9' | ||
| spec.add_dependency 'dotenv-rails', '~> 2.1' |
lib/europeana/feed_jobs.rb
Outdated
| require 'europeana/feed_jobs/engine' | ||
|
|
||
| module Europeana | ||
| module FeedJobs |
There was a problem hiding this comment.
I'm not convinced this is a great name for the Engine/gem/repo because there is already more than just the job in here. I think Europeana::Feeds would be better.
|
|
||
| module Europeana | ||
| module FeedJobs | ||
| class FeedJob < ActiveJob::Base |
There was a problem hiding this comment.
Rename the class to FetchJob to avoid the repetition with the module name.
| module Europeana | ||
| module FeedJobs | ||
| class FeedJob < ActiveJob::Base | ||
| queue_as :cache |
There was a problem hiding this comment.
I'd be tempted to name the queue after the Engine.
| # Global nav uses some feeds, and is cached so needs to be expired when those | ||
| # feeds are updated. | ||
| # Cached pages need to be expired should they be using the updated feed. | ||
| after_perform do |
There was a problem hiding this comment.
This does not belong here in the Engine.
I suggest:
- Removing the
after_performblock from here - Creating a new job class in europeana-portal-collections which sub-classes this one
- Adding the
after_performblock in to the app-specific job class
|
|
||
| def perform(url, download_media = false) | ||
| @url = url | ||
| @download_media = download_media |
There was a problem hiding this comment.
This does nothing inside the engine/gem itself and should be removed, along with the argument.
| @@ -1,14 +1,18 @@ | |||
| # frozen_string_literal: true | |||
There was a problem hiding this comment.
# frozen_string_literal: true needs to come after #!/usr/bin/env ruby, and the file should be made executable.
| { tag: :video, attr: :poster } | ||
| ].freeze | ||
|
|
||
| # @param entry [Feedjira::Parser::RSSEntry] |
|
@lbiedinger this looks fine to me now. Feel free to merge and create a first release when ready. |
No description provided.