From fcacb46d85f30766cfea3c7f11267ba58b4278fd Mon Sep 17 00:00:00 2001 From: Andrew Sullivan Cant Date: Sat, 27 Jun 2015 16:50:20 -0400 Subject: [PATCH] Extract Share and Initializer from Configuration The original motivation for this refactoring is to reduce the number of objects which are getting passed around making the Celluloid conversion easier. ActiveRecord objects (e.g., Share and Config) will be used from their class methods, more like Rails does. This will eliminate some object passing as well as deprecating some delegate code in the Configuration and Manager classes. This results in 3 classes instead of 1 and I hope will make each one easier to understand, and test. I have kept Configuration::Config to avoid changing the table names. This could be simplified in the future. --- bin/gitdocs | 1 + config.ru | 8 +- lib/gitdocs.rb | 2 + lib/gitdocs/cli.rb | 20 +-- lib/gitdocs/configuration.rb | 114 +++--------------- lib/gitdocs/initializer.rb | 63 ++++++++++ lib/gitdocs/manager.rb | 34 +----- .../migration/004_add_index_for_path.rb | 2 +- lib/gitdocs/repository.rb | 2 +- lib/gitdocs/server.rb | 5 +- lib/gitdocs/settings_app.rb | 8 +- lib/gitdocs/share.rb | 50 ++++++++ lib/gitdocs/views/settings.haml | 4 +- test/integration/test_helper.rb | 4 +- test/unit/configuration_test.rb | 78 ++---------- test/unit/share_test.rb | 68 +++++++++++ test/unit/test_helper.rb | 3 + 17 files changed, 248 insertions(+), 218 deletions(-) create mode 100644 lib/gitdocs/initializer.rb create mode 100644 lib/gitdocs/share.rb create mode 100644 test/unit/share_test.rb diff --git a/bin/gitdocs b/bin/gitdocs index 8a43600..9bb1f5c 100755 --- a/bin/gitdocs +++ b/bin/gitdocs @@ -2,4 +2,5 @@ require 'gitdocs' +Gitdocs::Initializer.initialize_all Gitdocs::Cli.start(ARGV) diff --git a/config.ru b/config.ru index 2a06ebc..28ced33 100644 --- a/config.ru +++ b/config.ru @@ -10,12 +10,12 @@ use Rack::Static, root: './lib/gitdocs/public' use Rack::MethodOverride -config_root = './tmp/web' -FileUtils.mkdir_p(config_root) -Gitdocs::SettingsApp.set :manager, Gitdocs::Manager.new(config_root, true) +Gitdocs::Initializer.root_dirname = './tmp/web' +Gitdocs::Initializer.initialize_database + Gitdocs::SettingsApp.set :logging, true map('/settings') { run Gitdocs::SettingsApp } -Gitdocs::BrowserApp.set :repositories, Gitdocs::Configuration.new(config_root).shares.map { |x| Gitdocs::Repository.new(x) } +Gitdocs::BrowserApp.set :repositories, Gitdocs::Share.all.map { |x| Gitdocs::Repository.new(x) } Gitdocs::BrowserApp.set :logging, true map('/') { run Gitdocs::BrowserApp } diff --git a/lib/gitdocs.rb b/lib/gitdocs.rb index a0dced7..d6fc817 100644 --- a/lib/gitdocs.rb +++ b/lib/gitdocs.rb @@ -10,6 +10,8 @@ require 'table_print' require 'gitdocs/version' +require 'gitdocs/initializer' +require 'gitdocs/share' require 'gitdocs/configuration' require 'gitdocs/runner' require 'gitdocs/server' diff --git a/lib/gitdocs/cli.rb b/lib/gitdocs/cli.rb index 95c33ec..52a1b8d 100644 --- a/lib/gitdocs/cli.rb +++ b/lib/gitdocs/cli.rb @@ -57,7 +57,7 @@ def restart method_option :pid, type: :string, aliases: '-P' desc 'add PATH', 'Adds a path to gitdocs' def add(path) - config.add_path(path) + Share.create_by_path!(normalize_path(path)) say "Added path #{path} to doc list" restart if running? end @@ -65,14 +65,14 @@ def add(path) method_option :pid, type: :string, aliases: '-P' desc 'rm PATH', 'Removes a path from gitdocs' def rm(path) - config.remove_path(path) + Share.remove_by_path(path) say "Removed path #{path} from doc list" restart if running? end desc 'clear', 'Clears all paths from gitdocs' def clear - config.clear + Share.destroy_all say 'Cleared paths from gitdocs' end @@ -103,7 +103,7 @@ def status status end tp( - config.shares, + Share.all, { sync: { display_method: :sync_type } }, { s: status_display }, :path @@ -120,7 +120,7 @@ def open end web_port = options[:port] - web_port ||= config.web_frontend_port + web_port ||= Configuration.web_frontend_port Launchy.open("http://localhost:#{web_port}/") end @@ -146,10 +146,6 @@ def runner ) end - def config - @config ||= Configuration.new - end - def running? runner.daemon_running? end @@ -162,6 +158,12 @@ def pid_path options[:pid] || '/tmp/gitdocs.pid' end + # @param [String] path + # @return [String] + def normalize_path(path) + File.expand_path(path, Dir.pwd) + end + # @return [Symbol] to indicate how the file system is being watched def file_system_watch_method # rubocop:disable CyclomaticComplexity if Guard::Listener.mac? && Guard::Darwin.usable? diff --git a/lib/gitdocs/configuration.rb b/lib/gitdocs/configuration.rb index 350f835..413fa86 100644 --- a/lib/gitdocs/configuration.rb +++ b/lib/gitdocs/configuration.rb @@ -1,112 +1,36 @@ # -*- encoding : utf-8 -*- require 'active_record' -require 'grit' class Gitdocs::Configuration - attr_reader :config_root - - def initialize(config_root = nil) - @config_root = config_root || File.expand_path('.gitdocs', ENV['HOME']) - FileUtils.mkdir_p(@config_root) - ActiveRecord::Base.establish_connection( - adapter: 'sqlite3', - database: ENV['TEST'] ? ':memory:' : File.join(@config_root, 'config.db') - ) - ActiveRecord::Migrator.migrate(File.expand_path('../migration', __FILE__)) - import_old_shares unless ENV['TEST'] - end - - class Share < ActiveRecord::Base - #attr_accessible :polling_interval, :path, :notification, :branch_name, :remote_name, :sync_type - end - - class Config < ActiveRecord::Base - #attr_accessible :start_web_frontend, :web_frontend_port - end - - # return [Boolean] - def start_web_frontend - global.start_web_frontend + # @return [Boolean] + def self.start_web_frontend + Config.global.start_web_frontend end # @return [Integer] - def web_frontend_port - global.web_frontend_port - end - - # @param [String] path - # @param [Hash] opts - def add_path(path, opts = nil) - path = normalize_path(path) - path_opts = { path: path } - path_opts.merge!(opts) if opts - Share.new(path_opts).save! + def self.web_frontend_port + Config.global.web_frontend_port end # @param [Hash] new_config - # @option new_config [Hash] 'config' - # @option new_config [Array] 'share' - def update_all(new_config) - global.update_attributes(new_config['config']) - new_config['share'].each do |index, share_config| - # Skip the share update if there is no path specified. - next unless share_config['path'] && !share_config['path'].empty? - - # Split the remote_branch into remote and branch - remote_branch = share_config.delete('remote_branch') - if remote_branch - share_config['remote_name'], share_config['branch_name'] = remote_branch.split('/', 2) - end - shares[index.to_i].update_attributes(share_config) - end + def self.update(new_config) + Config.global.update_attributes(new_config) end - # @param [String] path of the share to remove - def remove_path(path) - path = normalize_path(path) - Share.where(path: path).destroy_all - end - - # @param [Integer] id of the share to remove - # - # @return [true] share was deleted - # @return [false] share does not exist - def remove_by_id(id) - Share.find(id).destroy - true - rescue ActiveRecord::RecordNotFound - false - end - - def clear - Share.destroy_all - end - - def shares - Share.all - end - - ############################################################################## - - private - - def global - fail if Config.all.size > 1 - Config.create! if Config.all.empty? - Config.all.first - end - - def normalize_path(path) - File.expand_path(path, Dir.pwd) - end - - def import_old_shares - full_path = File.expand_path('paths', config_root) - return unless File.exist?(full_path) + # NOTE: This record has been kept as a subclass to avoid changing the + # database table. There are other ways to achieve this, but this seemed most + # clear for now. [2015-06-26 -- acant] + class Config < ActiveRecord::Base + # attr_accessible :start_web_frontend, :web_frontend_port - File.read(full_path).split("\n").each do |path| - Share.find_or_create_by_path(path) + # @return [Gitdocs::Configuration::Config] + def self.global + fail if all.size > 1 + create! if all.empty? + all.first end end end + + diff --git a/lib/gitdocs/initializer.rb b/lib/gitdocs/initializer.rb new file mode 100644 index 0000000..c48bbbc --- /dev/null +++ b/lib/gitdocs/initializer.rb @@ -0,0 +1,63 @@ +# -*- encoding : utf-8 -*- + +require 'active_record' + +module Gitdocs + class Initializer + # @return [nil] + def self.initialize_all + initialize_database + initialize_old_paths + end + + # @return [nil] + def self.initialize_database + FileUtils.mkdir_p(root_dirname) + ActiveRecord::Base.establish_connection( + adapter: 'sqlite3', + database: database + ) + ActiveRecord::Migrator.migrate( + File.expand_path('../migration', __FILE__) + ) + end + + # @return [nil] + def self.initialize_old_paths + old_path_dirname = File.expand_path('paths', root_dirname) + return unless File.exist?(old_path_dirname) + + File.read(old_path_dirname).split("\n").each do |path| + begin + Share.create_by_path!(path) + rescue # rubocop:disable ExceptionHandling + # Nothing to do, because we want the process to keep going. + end + end + end + + # @return [String] + def self.root_dirname + @root_dirname ||= File.expand_path('.gitdocs', ENV['HOME']) + end + + # @param [nil, String] value + # @return [nil] + def self.root_dirname=(value) + return if value.nil? + @root_dirname = value + end + + # @return [String] + def self.database + @database ||= File.join(root_dirname, 'config.db') + end + + # @param [nil, String] value + # @return [nil] + def self.database=(value) + return if value.nil? + @database = value + end + end +end diff --git a/lib/gitdocs/manager.rb b/lib/gitdocs/manager.rb index 8c3b0a9..206c9c7 100644 --- a/lib/gitdocs/manager.rb +++ b/lib/gitdocs/manager.rb @@ -4,18 +4,19 @@ module Gitdocs Restart = Class.new(RuntimeError) class Manager - attr_reader :config, :debug + attr_reader :debug def initialize(config_root, debug) - @config = Configuration.new(config_root) - @logger = Logger.new(File.expand_path('log', @config.config_root)) - @debug = debug + Initializer.root_dirname = config_root + @logger = Logger.new(File.expand_path('log', Initializer.root_dirname)) + @debug = debug yield @config if block_given? end def start(web_port = nil) log("Starting Gitdocs v#{VERSION}...") - log("Using configuration root: '#{config.config_root}'") + log("Using configuration root: '#{Initializer.root_dirname}'") + shares = Share.all log("Shares: (#{shares.length}) #{shares.map(&:inspect).join(', ')}") begin @@ -59,28 +60,5 @@ def stop def log(msg, level = :info) @debug ? puts(msg) : @logger.send(level, msg) end - - def start_web_frontend - config.start_web_frontend - end - - def web_frontend_port - config.web_frontend_port - end - - def shares - config.shares - end - - # @see Gitdocs::Configuration#update_all - def update_all(new_config) - config.update_all(new_config) - EM.add_timer(0.1) { restart } - end - - # @see Gitdocs::Configuration#remove_by_id - def remove_by_id(id) - config.remove_by_id(id) - end end end diff --git a/lib/gitdocs/migration/004_add_index_for_path.rb b/lib/gitdocs/migration/004_add_index_for_path.rb index 726b041..1444d2b 100644 --- a/lib/gitdocs/migration/004_add_index_for_path.rb +++ b/lib/gitdocs/migration/004_add_index_for_path.rb @@ -4,7 +4,7 @@ class AddIndexForPath < ActiveRecord::Migration def self.up - shares = Gitdocs::Configuration::Share.all.reduce(Hash.new { |h, k| h[k] = [] }) { |h, s| h[s.path] << s; h } + shares = Gitdocs::Share.all.reduce(Hash.new { |h, k| h[k] = [] }) { |h, s| h[s.path] << s; h } shares.each do |path, shares| shares.shift shares.each(&:destroy) unless shares.empty? diff --git a/lib/gitdocs/repository.rb b/lib/gitdocs/repository.rb index ab4edeb..521034c 100644 --- a/lib/gitdocs/repository.rb +++ b/lib/gitdocs/repository.rb @@ -21,7 +21,7 @@ class Gitdocs::Repository # @see #valid? # @see #invalid_reason # - # @param [String, Configuration::Share] path_or_share + # @param [String, Share] path_or_share def initialize(path_or_share) path = path_or_share if path_or_share.respond_to?(:path) diff --git a/lib/gitdocs/server.rb b/lib/gitdocs/server.rb index 8b90cf3..81f8093 100644 --- a/lib/gitdocs/server.rb +++ b/lib/gitdocs/server.rb @@ -14,9 +14,9 @@ def initialize(manager, port = 8888, repositories) end def self.start_and_wait(manager, override_port, repositories) - return false unless manager.start_web_frontend + return false unless Configuration.start_web_frontend - web_port = override_port || manager.web_frontend_port + web_port = override_port || Configuration.web_frontend_port server = Server.new(manager, web_port, repositories) server.start server.wait_for_start @@ -24,7 +24,6 @@ def self.start_and_wait(manager, override_port, repositories) end def start - Gitdocs::SettingsApp.set :manager, @manager Gitdocs::BrowserApp.set :repositories, @repositories Thin::Logging.debug = @manager.debug diff --git a/lib/gitdocs/settings_app.rb b/lib/gitdocs/settings_app.rb index b128dc1..169dd27 100644 --- a/lib/gitdocs/settings_app.rb +++ b/lib/gitdocs/settings_app.rb @@ -10,18 +10,20 @@ class SettingsApp < Sinatra::Base get('/') do haml( :settings, - locals: { conf: settings.manager, nav_state: 'settings' } + locals: { nav_state: 'settings' } ) end post('/') do - settings.manager.update_all(request.POST) + Configuration.update(request.POST['config']) + Share.update_all(request.POST['share']) + EM.add_timer(0.1) { Gitdocs.restart } redirect to('/') end delete('/:id') do id = params[:id].to_i - halt(404) unless settings.manager.remove_by_id(id) + halt(404) unless Share.remove_by_id(id) redirect to('/') end end diff --git a/lib/gitdocs/share.rb b/lib/gitdocs/share.rb new file mode 100644 index 0000000..0d932c9 --- /dev/null +++ b/lib/gitdocs/share.rb @@ -0,0 +1,50 @@ +# -*- encoding : utf-8 -*- + +require 'active_record' + +class Gitdocs::Share < ActiveRecord::Base + # @param [#to_i] index + # + # @return [Share] + def self.at(index) + all[index.to_i] + end + + # @param [String] path + def self.create_by_path!(path) + new(path: File.expand_path(path)).save! + end + + # @param [Hash] updated_shares + # @return [void] + def self.update_all(updated_shares) + updated_shares.each do |index, share_config| + # Skip the share update if there is no path specified. + next unless share_config['path'] && !share_config['path'].empty? + + # Split the remote_branch into remote and branch + remote_branch = share_config.delete('remote_branch') + share_config['remote_name'], share_config['branch_name'] = + remote_branch.split('/', 2) if remote_branch + + at(index).update_attributes(share_config) + end + end + + # @param [Integer] id of the share to remove + # + # @return [true] share was deleted + # @return [false] share does not exist + def self.remove_by_id(id) + find(id).destroy + true + rescue ActiveRecord::RecordNotFound + false + end + + # @param [String] path of the share to remove + # @return [void] + def self.remove_by_path(path) + where(path: File.expand_path(path)).destroy_all + end +end diff --git a/lib/gitdocs/views/settings.haml b/lib/gitdocs/views/settings.haml index 60dea3c..f3fa445 100644 --- a/lib/gitdocs/views/settings.haml +++ b/lib/gitdocs/views/settings.haml @@ -7,9 +7,9 @@ %dl %dt Web Frontend Port %dd - %input{ type: 'input', name: 'config[web_frontend_port]', value: conf.web_frontend_port } + %input{ type: 'input', name: 'config[web_frontend_port]', value: Gitdocs::Configuration.web_frontend_port } %h2 Shares - - conf.shares.each_with_index do |share, idx| + - Gitdocs::Share.all.each_with_index do |share, idx| .share{ id: "share-#{idx}", class: idx.even? ? 'even' : 'odd' } %dl %dt Path diff --git a/test/integration/test_helper.rb b/test/integration/test_helper.rb index 8b63bf6..5b666ad 100644 --- a/test/integration/test_helper.rb +++ b/test/integration/test_helper.rb @@ -90,8 +90,8 @@ def after_teardown end def start_daemon - configuration = Gitdocs::Configuration.new - configuration.shares.each do |share| + Gitdocs::Initializer.initialize_database + Gitdocs::Share.all.each do |share| share.update_attributes(polling_interval: 0.1, notification: false) end diff --git a/test/unit/configuration_test.rb b/test/unit/configuration_test.rb index 28c9b9d..56203f1 100644 --- a/test/unit/configuration_test.rb +++ b/test/unit/configuration_test.rb @@ -2,81 +2,19 @@ require File.expand_path('../test_helper', __FILE__) -describe 'gitdocs configuration' do +describe Gitdocs::Configuration do before do - ENV['TEST'] = 'true' - ShellTools.capture { @config = Gitdocs::Configuration.new('/tmp/gitdocs') } + ShellTools.capture { Gitdocs::Initializer.initialize_database } end - it 'has sensible default config root' do - assert_equal '/tmp/gitdocs', @config.config_root - end - - it 'can retrieve empty shares' do - assert_equal [], @config.shares - end - - it 'can have a path added' do - @config.add_path('/my/../my/path') # normalized test - assert_equal '/my/path', @config.shares.first.path - assert_equal 15.0, @config.shares.first.polling_interval - end - - it 'can have a path removed' do - @config.add_path('/my/path') - @config.add_path('/my/path/2') - @config.remove_path('/my/../my/path/2') # normalized test - assert_equal ['/my/path'], @config.shares.map(&:path) - end - - it 'can have a share removed by id' do - @config.add_path('/my/path') - @config.add_path('/my/path/2') - - # Delete an index which exists - @config.remove_by_id(2).must_equal(true) # normalized test - assert_equal ['/my/path'], @config.shares.map(&:path) - - # Try to delete an index which does not exist - @config.remove_by_id(5).must_equal(false) # normalized test - assert_equal ['/my/path'], @config.shares.map(&:path) - end - - it 'can clear paths' do - @config.add_path('/my/path') - @config.add_path('/my/path/2') - @config.clear - assert_equal [], @config.shares.map(&:path) - end - - describe '#update_all' do + describe 'Config.update' do before do - @config.add_path('/my/path') - @config.add_path('/my/path/2') - @config.add_path('/my/path/3') - @config.add_path('/my/path/4') - @config.add_path('/my/path/5') - @config.update_all( - 'config' => { 'start_web_frontend' => false, 'web_frontend_port' => 9999 }, - 'share' => { - '0' => { 'path' => '/my/path', 'polling_interval' => 42 }, - '1' => { 'path' => '/my/path/2', 'polling_interval' => 66 }, - '2' => { 'path' => '/my/path/3a', 'polling_interval' => 77 }, - '3' => { 'path' => '', 'polling_interval' => 88 }, - '4' => { 'polling_interval' => 99 } - } + Gitdocs::Configuration.update( + 'start_web_frontend' => false, 'web_frontend_port' => 9999 ) end - it { @config.shares.size.must_equal(5) } - it { @config.shares[0].path.must_equal('/my/path') } - it { @config.shares[0].polling_interval.must_equal(42) } - it { @config.shares[1].path.must_equal('/my/path/2') } - it { @config.shares[1].polling_interval.must_equal(66) } - it { @config.shares[2].path.must_equal('/my/path/3a') } - it { @config.shares[2].polling_interval.must_equal(77) } - it { @config.shares[3].path.must_equal('/my/path/4') } - it { @config.shares[3].polling_interval.must_equal(15) } - it { @config.shares[4].path.must_equal('/my/path/5') } - it { @config.shares[4].polling_interval.must_equal(15) } + + it { Gitdocs::Configuration.start_web_frontend.must_equal(false) } + it { Gitdocs::Configuration.web_frontend_port.must_equal(9999) } end end diff --git a/test/unit/share_test.rb b/test/unit/share_test.rb new file mode 100644 index 0000000..fae3f2f --- /dev/null +++ b/test/unit/share_test.rb @@ -0,0 +1,68 @@ +# -*- encoding : utf-8 -*- + +require File.expand_path('../test_helper', __FILE__) + +describe Gitdocs::Share do + before do + ShellTools.capture { Gitdocs::Initializer.initialize_database } + end + + it 'can retrieve empty shares' do + assert_equal [], Gitdocs::Share.all.to_a + end + + it 'can have a path added' do + Gitdocs::Share.create_by_path!('/my/../my/path') # normalized test + assert_equal '/my/path', Gitdocs::Share.at(0).path + assert_equal 15.0, Gitdocs::Share.at(0).polling_interval + end + + it 'can have a path removed' do + Gitdocs::Share.create_by_path!('/my/path') + Gitdocs::Share.create_by_path!('/my/path/2') + Gitdocs::Share.remove_by_path('/my/../my/path/2') # normalized test + assert_equal ['/my/path'], Gitdocs::Share.all.map(&:path) + end + + it 'can have a share removed by id' do + Gitdocs::Share.create_by_path!('/my/path') + Gitdocs::Share.create_by_path!('/my/path/2') + + # Delete an index which exists + Gitdocs::Share.remove_by_id(2).must_equal(true) # normalized test + assert_equal ['/my/path'], Gitdocs::Share.all.map(&:path) + + # Try to delete an index which does not exist + Gitdocs::Share.remove_by_id(5).must_equal(false) # normalized test + assert_equal ['/my/path'], Gitdocs::Share.all.map(&:path) + end + + describe '#update_all' do + before do + Gitdocs::Share.create_by_path!('/my/path') + Gitdocs::Share.create_by_path!('/my/path/2') + Gitdocs::Share.create_by_path!('/my/path/3') + Gitdocs::Share.create_by_path!('/my/path/4') + Gitdocs::Share.create_by_path!('/my/path/5') + + Gitdocs::Share.update_all( + '0' => { 'path' => '/my/path', 'polling_interval' => 42 }, + '1' => { 'path' => '/my/path/2', 'polling_interval' => 66 }, + '2' => { 'path' => '/my/path/3a', 'polling_interval' => 77 }, + '3' => { 'path' => '', 'polling_interval' => 88 }, + '4' => { 'polling_interval' => 99 } + ) + end + it { Gitdocs::Share.all.size.must_equal(5) } + it do + Gitdocs::Share.all.map(&:path).must_equal( + %w(/my/path /my/path/2 /my/path/3a /my/path/4 /my/path/5) + ) + end + it do + Gitdocs::Share.all.map(&:polling_interval).must_equal( + [42, 66, 77, 15, 15] + ) + end + end +end diff --git a/test/unit/test_helper.rb b/test/unit/test_helper.rb index b14ea00..de077c1 100644 --- a/test/unit/test_helper.rb +++ b/test/unit/test_helper.rb @@ -6,5 +6,8 @@ require 'gitdocs' require 'mocha/setup' +Gitdocs::Initializer.root_dirname = '/tmp/gitdocs' +Gitdocs::Initializer.database = ':memory:' + require 'coveralls' Coveralls.wear!