Refactoring migrators into importers #575

Closed
wants to merge 22 commits into
from

Conversation

Projects
None yet
3 participants
Contributor

tombell commented Jun 13, 2012

This pull request is based upon the work that @mojombo and I started working on refactoring the existing migrators, adding some structure and testability. It is also currently work in progress.

General

Importers are simple classes in the Jekyll::Importers namespace, that extend Importer and implement 3 methods.

  • self.help - return a string of the usage and command line options required
  • self.validate - validate and return an array of error messages for command line options
  • self.process - process the import and return a specific data structure
module Jekyll
  module Importers
    class CSV < Importer
      def self.help
        # ...
      end

      def self.validate(options)
        # ...
      end

      def self.process(options)
        # ...
      end
    end
  end
end

Help

The self.help method basically returns a string which will be used for the help message and if zero options are passed.

Validation

The self.validate method will check that the required options are present and valid in the options hash passed to the method. Should there be any errors, the messages for these errors should be pushed into an array that is returned by self.validate.

Processing

The self.process method will process the import and return a hash that the write_files method in Importer will use to write all the different files.

File Hash

Each file to be written is represent with it's own hash with specific keys.

{
  :name => 'the-file-name.md',
  :body => 'The contents to be written',

  :header => {
    :layout => 'post',
    :title => 'The Title'
  }
}

Files Hash

This is the hash that is going to be returned from the self.process method. Some importers may wish to import other types of files like pages from WordPress. Just specify another key in the array with the type of file and include an array of file hashes. The type will also determine which directory the files are written into.

{
  :posts => posts
  # :pages => pages
}
Contributor

tombell commented Jun 20, 2012

Finished converting all the migrators into new importers.

I've simplified a lot of ones which had a ton of options, if people require the extra options, might be worth opening a future pull request. I didn't want to pollute the opts in bin/jekyll with a ton of options for importers.

@calavera calavera and 1 other commented on an outdated diff Jul 20, 2012

bin/jekyll
- if Jekyll.const_defined?(migrators[migrator.to_sym])
- migrator_class = Jekyll.const_get(migrators[migrator.to_sym])
- migrator_class.process(*cmd_options)
+ puts "Importing..."
+
+ files = migrator_class.process(cmd_options)
+ migrator_class.write_files(files)
+
+ files.each do |type, specs|
+ puts "Imported #{type}: #{specs.size}"
+ end
else
puts "Invalid migrator. Run `jekyll --help` for assistance."
@calavera

calavera Jul 20, 2012

Contributor

If we already have the list of available migrators, it should be nice to print them, so people don't really need to run jekyll --help to check the migrator's name. Something like this would be more helpful:

puts "Invalid migrator #{migrator}."
puts "This is the list of available migrators:"
puts migrators.keys.map {|m| "\t- #{m}\n"}
@tombell

tombell Jul 20, 2012

Contributor

Would you say that reading the migrators from the directory itself is the best way to keep a list of available adapters to handle this?

@calavera

calavera Jul 20, 2012

Contributor

we already have the migrators in the hashmap above, so I don't see it necessary. I rather be explicit with what's loaded, although I see how the actual implementation can be a problem if you let people to add new importers because they have to modify the binary too.

@tombell

tombell Jul 20, 2012

Contributor

Can just use the above hash map until it becomes an issue with an increasing amount of migrators I guess.

@calavera

calavera Jul 20, 2012

Contributor

I'd totally do that

@tombell

tombell Jul 20, 2012

Contributor

There was also a bug that it tried requiring a migrator before it even checked if it was an available one, which I've fixed alongside displaying the list of available adapters.

@calavera calavera commented on the diff Jul 20, 2012

lib/jekyll/importer.rb
@@ -0,0 +1,23 @@
+require 'fileutils'
+
+module Jekyll
+ class Importer
+ def self.write_files(files)
+ files.keys.each { |key| FileUtils.mkdir_p "_#{key}" }
@calavera

calavera Jul 20, 2012

Contributor

FileUtils.mkdir_p accepts a list, so this can be rewritten as:

FileUtils.mkdir_p files.keys
@tombell

tombell Jul 20, 2012

Contributor

The directory needs the _ at the start of the directory name, so I think there is going to be a map here anyway, I think.

@calavera

calavera Jul 20, 2012

Contributor

oh, yes, sorry, I didn't notice the _

@calavera calavera commented on an outdated diff Jul 20, 2012

lib/jekyll/importers/csv.rb
@@ -0,0 +1,97 @@
+begin
+ require 'fastercsv' unless RUBY_VERSION > '1.9'
+ require 'csv' if RUBY_VERSION > '1.9'
+rescue LoadError
+ puts 'FasterCSV gem is not installed. Please do `[sudo] gem install fastercsv`'
+ exit(1)
@calavera

calavera Jul 20, 2012

Contributor

use abort rather than exit(1) when it's possible. You can pass it an string an it's way nicer:

abort 'FasterCSV gem is not installed. Please do `[sudo] gem install fastercsv`'

tombell added some commits Jul 20, 2012

@tombell tombell Use abort instead of exit f24f86e
@tombell tombell Fix the detection of invalid migrators
* Detect an invalid migrator if it's not in the hash map
* Display a list of available migrators using the hash map
a7c80cd

tombell closed this Aug 10, 2012

jekyllbot locked and limited conversation to collaborators Feb 27, 2017

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.