From 5fd54d0ecc78cb807558c80628c2af7e3af10bbd Mon Sep 17 00:00:00 2001
From: Alexis Reigel <mail@koffeinfrei.org>
Date: Fri, 1 Jun 2018 11:40:55 +0200
Subject: [PATCH] add rubocop and fix issues
---
.gitlab-ci.yml | 12 ++++--
.rubocop.yml | 10 +++++
Gemfile | 6 ++-
Gemfile.lock | 16 ++++++++
Rakefile | 14 ++++---
bin/console | 7 ++--
excelsior.gemspec | 40 ++++++++++--------
lib/excelsior.rb | 4 +-
lib/excelsior/error.rb | 2 +
lib/excelsior/import.rb | 49 +++++++++++++---------
lib/excelsior/mapping.rb | 5 ++-
lib/excelsior/report.rb | 2 +
lib/excelsior/source.rb | 2 +
lib/excelsior/transaction.rb | 2 +
lib/excelsior/version.rb | 4 +-
test/excelsior_test.rb | 80 +++++++++++++++++++-----------------
test/test_helper.rb | 16 +++++---
17 files changed, 171 insertions(+), 100 deletions(-)
create mode 100644 .rubocop.yml
diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
index ce790bc..d80624c 100644
--- a/.gitlab-ci.yml
+++ b/.gitlab-ci.yml
@@ -4,9 +4,15 @@ cache:
paths:
- vendor/
+before_script:
+ - bundle install -j $(nproc) --path vendor
+
test:
stage: test
script:
- - apt-get update -qy
- - bundle install --path vendor
- - bundle exec rake test
+ - bundle exec rake test
+
+rubocop:
+ stage: test
+ script:
+ - bundle exec rubocop
diff --git a/.rubocop.yml b/.rubocop.yml
new file mode 100644
index 0000000..e125078
--- /dev/null
+++ b/.rubocop.yml
@@ -0,0 +1,10 @@
+Metrics/LineLength:
+ Max: 100
+
+# exclude for tests
+Metrics/BlockLength:
+ ExcludedMethods: ['describe', 'it']
+
+# don't require documentation on classes / modules
+Style/Documentation:
+ Enabled: false
diff --git a/Gemfile b/Gemfile
index 232142a..5556ddc 100644
--- a/Gemfile
+++ b/Gemfile
@@ -1,6 +1,8 @@
-source "https://rubygems.org"
+# frozen_string_literal: true
-git_source(:github) {|repo_name| "https://github.com/#{repo_name}" }
+source 'https://rubygems.org'
+
+git_source(:github) { |repo_name| "https://github.com/#{repo_name}" }
# Specify your gem's dependencies in excelsior.gemspec
gemspec
diff --git a/Gemfile.lock b/Gemfile.lock
index 9b181ca..be055d2 100644
--- a/Gemfile.lock
+++ b/Gemfile.lock
@@ -51,6 +51,7 @@ GEM
minitest (~> 5.1)
tzinfo (~> 1.1)
arel (9.0.0)
+ ast (2.4.0)
builder (3.2.3)
concurrent-ruby (1.0.5)
crass (1.0.4)
@@ -74,6 +75,10 @@ GEM
nio4r (2.3.1)
nokogiri (1.8.2)
mini_portile2 (~> 2.3.0)
+ parallel (1.12.1)
+ parser (2.5.1.0)
+ ast (~> 2.4.0)
+ powerpack (0.1.1)
rack (2.0.5)
rack-test (1.0.0)
rack (>= 1.0, < 3)
@@ -101,7 +106,16 @@ GEM
method_source
rake (>= 0.8.7)
thor (>= 0.18.1, < 2.0)
+ rainbow (3.0.0)
rake (10.5.0)
+ rubocop (0.56.0)
+ parallel (~> 1.10)
+ parser (>= 2.5)
+ powerpack (~> 0.1)
+ rainbow (>= 2.2.2, < 4.0)
+ ruby-progressbar (~> 1.7)
+ unicode-display_width (~> 1.0, >= 1.0.1)
+ ruby-progressbar (1.9.0)
rubyzip (1.2.1)
simple_xlsx_reader (1.0.2)
nokogiri
@@ -118,6 +132,7 @@ GEM
thread_safe (0.3.6)
tzinfo (1.2.5)
thread_safe (~> 0.1)
+ unicode-display_width (1.3.3)
websocket-driver (0.7.0)
websocket-extensions (>= 0.1.0)
websocket-extensions (0.1.3)
@@ -130,6 +145,7 @@ DEPENDENCIES
excelsior!
minitest (~> 5.0)
rake (~> 10.0)
+ rubocop (~> 0.56.0)
sqlite3
BUNDLED WITH
diff --git a/Rakefile b/Rakefile
index 66d80bc..43290b2 100644
--- a/Rakefile
+++ b/Rakefile
@@ -1,11 +1,13 @@
-require "bundler/gem_tasks"
-require "rake/testtask"
+# frozen_string_literal: true
+
+require 'bundler/gem_tasks'
+require 'rake/testtask'
Rake::TestTask.new(:test) do |t|
- t.libs << "test"
- t.libs << "lib"
- t.test_files = FileList["test/**/*_test.rb"]
+ t.libs << 'test'
+ t.libs << 'lib'
+ t.test_files = FileList['test/**/*_test.rb']
t.options = '-b'
end
-task :default => :test
+task default: :test
diff --git a/bin/console b/bin/console
index 03b778f..2b65065 100755
--- a/bin/console
+++ b/bin/console
@@ -1,7 +1,8 @@
#!/usr/bin/env ruby
+# frozen_string_literal: true
-require "bundler/setup"
-require "excelsior"
+require 'bundler/setup'
+require 'excelsior'
# You can add fixtures and/or initialization code here to make experimenting
# with your gem easier. You can also use a different console, if you like.
@@ -10,5 +11,5 @@ require "excelsior"
# require "pry"
# Pry.start
-require "irb"
+require 'irb'
IRB.start(__FILE__)
diff --git a/excelsior.gemspec b/excelsior.gemspec
index a449320..855a978 100644
--- a/excelsior.gemspec
+++ b/excelsior.gemspec
@@ -1,31 +1,35 @@
-lib = File.expand_path("../lib", __FILE__)
+# frozen_string_literal: true
+
+lib = File.expand_path('lib', __dir__)
$LOAD_PATH.unshift(lib) unless $LOAD_PATH.include?(lib)
-require "excelsior/version"
+require 'excelsior/version'
Gem::Specification.new do |spec|
- spec.name = "excelsior"
+ spec.name = 'excelsior'
spec.version = Excelsior::VERSION
- spec.authors = ["Immanuel Häussermann"]
- spec.email = ["hai@panter.ch"]
+ spec.authors = ['Immanuel Häussermann']
+ spec.email = ['hai@panter.ch']
- spec.summary = %q{Helps you import data from an excel sheet}
- spec.description = %q{Provides a concise DSL to map, validate and import data from an excel sheet into your ruby app}
- spec.homepage = "http://github.com/manufaktor/excelsior"
- spec.license = "MIT"
+ spec.summary = 'Helps you import data from an excel sheet'
+ spec.description = 'Provides a concise DSL to map, validate and import data ' \
+ 'from an excel sheet into your ruby app'
+ spec.homepage = 'http://github.com/manufaktor/excelsior'
+ spec.license = 'MIT'
spec.files = `git ls-files -z`.split("\x0").reject do |f|
f.match(%r{^(test|spec|features)/})
end
- spec.bindir = "exe"
+ spec.bindir = 'exe'
spec.executables = spec.files.grep(%r{^exe/}) { |f| File.basename(f) }
- spec.require_paths = ["lib"]
+ spec.require_paths = ['lib']
- spec.add_development_dependency "bundler", "~> 1.16"
- spec.add_development_dependency "rake", "~> 10.0"
- spec.add_development_dependency "minitest", "~> 5.0"
- spec.add_development_dependency "sqlite3"
+ spec.add_development_dependency 'bundler', '~> 1.16'
+ spec.add_development_dependency 'minitest', '~> 5.0'
+ spec.add_development_dependency 'rake', '~> 10.0'
+ spec.add_development_dependency 'rubocop', '~> 0.56.0'
+ spec.add_development_dependency 'sqlite3'
- spec.add_runtime_dependency "simple_xlsx_reader", "~> 1.0.2"
- spec.add_runtime_dependency "rails", ">= 4.0.0"
- spec.add_runtime_dependency "activerecord", ">= 4.0.0"
+ spec.add_runtime_dependency 'activerecord', '>= 4.0.0'
+ spec.add_runtime_dependency 'rails', '>= 4.0.0'
+ spec.add_runtime_dependency 'simple_xlsx_reader', '~> 1.0.2'
end
diff --git a/lib/excelsior.rb b/lib/excelsior.rb
index 3e35195..51f6da2 100644
--- a/lib/excelsior.rb
+++ b/lib/excelsior.rb
@@ -1,4 +1,6 @@
-require "excelsior/version"
+# frozen_string_literal: true
+
+require 'excelsior/version'
module Excelsior
end
diff --git a/lib/excelsior/error.rb b/lib/excelsior/error.rb
index 8493619..aefa2b9 100644
--- a/lib/excelsior/error.rb
+++ b/lib/excelsior/error.rb
@@ -1,3 +1,5 @@
+# frozen_string_literal: true
+
module Excelsior
Error = Struct.new(:row, :errors)
end
diff --git a/lib/excelsior/import.rb b/lib/excelsior/import.rb
index d4e0527..c2525f1 100644
--- a/lib/excelsior/import.rb
+++ b/lib/excelsior/import.rb
@@ -1,3 +1,5 @@
+# frozen_string_literal: true
+
require 'simple_xlsx_reader'
require 'rails'
require 'active_record'
@@ -20,7 +22,7 @@ module Excelsior
self.source = file || self.class.source_file
self.fields = self.class.fields
- @doc = ::SimpleXlsxReader.open(self.source)
+ @doc = ::SimpleXlsxReader.open(source)
@sheet = @doc.sheets.first
@columns = @sheet.rows.shift
@@ -36,7 +38,7 @@ module Excelsior
model_class.transaction do
insert_rows(&block)
- raise ActiveRecord::Rollback if @report.failed > 0
+ raise ActiveRecord::Rollback if @report.failed.positive?
end
else
insert_rows(&block)
@@ -44,20 +46,17 @@ module Excelsior
end
def valid?
- @errors = fields.to_a.reduce({}) do |acc, f|
+ @errors = fields.to_a.each_with_object({}) do |f, acc|
acc[:missing_column] ||= []
- unless @columns.include?(f[:header])
- acc[:missing_column] << { missing: f[:header] }
- end
- acc
+ acc[:missing_column] << { missing: f[:header] } unless @columns.include?(f[:header])
end
end
private
def model_class
- self.class.name.gsub("Import", "").constantize
+ self.class.name.gsub('Import', '').constantize
end
def add_model_errors(record, index)
@@ -84,19 +83,29 @@ module Excelsior
def insert_rows(&block)
@rows.map.with_index do |row, i|
attributes = map_row_values(row, @columns)
- if block_given?
- begin
- result = block.call(attributes)
- report_insert
- result
- rescue
- report_failure
- end
- else
- record = model_class.create(attributes)
- add_model_errors(record, i)
- end
+ insert_row(attributes, i, &block)
+ end
+ end
+
+ def insert_row(attributes, index, &block)
+ if block_given?
+ insert_row_with_block(attributes, &block)
+ else
+ insert_row_without_block(attributes, index)
end
end
+
+ def insert_row_with_block(attributes)
+ result = yield(attributes)
+ report_insert
+ result
+ rescue StandardError
+ report_failure
+ end
+
+ def insert_row_without_block(attributes, index)
+ record = model_class.create(attributes)
+ add_model_errors(record, index)
+ end
end
end
diff --git a/lib/excelsior/mapping.rb b/lib/excelsior/mapping.rb
index 12b9035..d41ccee 100644
--- a/lib/excelsior/mapping.rb
+++ b/lib/excelsior/mapping.rb
@@ -1,3 +1,5 @@
+# frozen_string_literal: true
+
module Excelsior
module Mapping
def self.included(host_class)
@@ -17,10 +19,9 @@ module Excelsior
end
def map_row_values(row, columns)
- @fields.to_a.reduce({}) do |acc, field|
+ @fields.to_a.each_with_object({}) do |field, acc|
idx = columns.index(field[:header])
acc[field[:attribute]] = row[idx]
- acc
end
end
end
diff --git a/lib/excelsior/report.rb b/lib/excelsior/report.rb
index ce93fca..616fbce 100644
--- a/lib/excelsior/report.rb
+++ b/lib/excelsior/report.rb
@@ -1,3 +1,5 @@
+# frozen_string_literal: true
+
module Excelsior
Report = Struct.new(:inserted, :failed) do
def initialize(*args)
diff --git a/lib/excelsior/source.rb b/lib/excelsior/source.rb
index fbbe731..8b9648b 100644
--- a/lib/excelsior/source.rb
+++ b/lib/excelsior/source.rb
@@ -1,3 +1,5 @@
+# frozen_string_literal: true
+
module Excelsior
module Source
def self.included(host_class)
diff --git a/lib/excelsior/transaction.rb b/lib/excelsior/transaction.rb
index d3a2247..e0950e7 100644
--- a/lib/excelsior/transaction.rb
+++ b/lib/excelsior/transaction.rb
@@ -1,3 +1,5 @@
+# frozen_string_literal: true
+
module Excelsior
module Transaction
def self.included(host_class)
diff --git a/lib/excelsior/version.rb b/lib/excelsior/version.rb
index 8c605ac..5e6ab00 100644
--- a/lib/excelsior/version.rb
+++ b/lib/excelsior/version.rb
@@ -1,3 +1,5 @@
+# frozen_string_literal: true
+
module Excelsior
- VERSION = "0.1.0"
+ VERSION = '0.1.0'
end
diff --git a/test/excelsior_test.rb b/test/excelsior_test.rb
index 259dc2d..2d97bb3 100644
--- a/test/excelsior_test.rb
+++ b/test/excelsior_test.rb
@@ -1,5 +1,7 @@
-require "test_helper"
-require "excelsior/import"
+# frozen_string_literal: true
+
+require 'test_helper'
+require 'excelsior/import'
class User < ActiveRecord::Base
validates :first_name, presence: true
@@ -10,11 +12,11 @@ describe Excelsior do
Object.send(:remove_const, :UserImport) if Object.constants.include?(:UserImport)
class UserImport < Excelsior::Import
- source "test/files/complete.xlsx"
+ source 'test/files/complete.xlsx'
- map "Vorname", to: :first_name
- map "Nachname", to: :last_name
- map "E-Mail", to: :email
+ map 'Vorname', to: :first_name
+ map 'Nachname', to: :last_name
+ map 'E-Mail', to: :email
end
@import = UserImport.new
@@ -27,19 +29,19 @@ describe Excelsior do
describe 'source' do
it 'allows setting the source on the class' do
import = UserImport.new
- assert_equal "test/files/complete.xlsx", import.source
+ assert_equal 'test/files/complete.xlsx', import.source
end
it 'allows setting the source on an instance level' do
- import = UserImport.new("test/files/missing-column.xlsx")
- assert_equal "test/files/missing-column.xlsx", import.source
+ import = UserImport.new('test/files/missing-column.xlsx')
+ assert_equal 'test/files/missing-column.xlsx', import.source
end
end
describe 'transaction' do
describe 'transaction is not set' do
it 'inserts only the valid rows' do
- UserImport.new("test/files/missing-first-name.xlsx").run
+ UserImport.new('test/files/missing-first-name.xlsx').run
assert_equal 2, User.count
end
end
@@ -49,17 +51,17 @@ describe Excelsior do
Object.send(:remove_const, :UserImport) if Object.constants.include?(:UserImport)
class UserImport < Excelsior::Import
- source "test/files/complete.xlsx"
+ source 'test/files/complete.xlsx'
transaction true
- map "Vorname", to: :first_name
- map "Nachname", to: :last_name
- map "E-Mail", to: :email
+ map 'Vorname', to: :first_name
+ map 'Nachname', to: :last_name
+ map 'E-Mail', to: :email
end
end
describe 'without a block' do
- let(:import) { UserImport.new("test/files/missing-first-name.xlsx") }
+ let(:import) { UserImport.new('test/files/missing-first-name.xlsx') }
before do
import.run
@@ -78,7 +80,7 @@ describe Excelsior do
describe 'with a block' do
it 'rolls back if one record raises an error' do
- UserImport.new("test/files/missing-first-name.xlsx").run do |v|
+ UserImport.new('test/files/missing-first-name.xlsx').run do |v|
User.create!(v)
end
@@ -86,7 +88,7 @@ describe Excelsior do
end
it 'does not roll back if the block does not raise an error' do
- UserImport.new("test/files/missing-first-name.xlsx").run do |v|
+ UserImport.new('test/files/missing-first-name.xlsx').run do |v|
User.create(v)
end
@@ -105,18 +107,18 @@ describe Excelsior do
describe '#columns' do
it 'returns the columns as defined in the excel' do
assert_equal 3, @import.columns.length
- assert_equal "E-Mail", @import.columns[0]
- assert_equal "Vorname", @import.columns[1]
- assert_equal "Nachname", @import.columns[2]
+ assert_equal 'E-Mail', @import.columns[0]
+ assert_equal 'Vorname', @import.columns[1]
+ assert_equal 'Nachname', @import.columns[2]
end
end
describe '#fields' do
it 'returns the mapped fields' do
assert_equal @import.fields, [
- { attribute: :first_name, header: "Vorname" },
- { attribute: :last_name, header: "Nachname" },
- { attribute: :email, header: "E-Mail" }
+ { attribute: :first_name, header: 'Vorname' },
+ { attribute: :last_name, header: 'Nachname' },
+ { attribute: :email, header: 'E-Mail' }
]
end
end
@@ -132,28 +134,30 @@ describe Excelsior do
describe 'with a block' do
it 'yields the records to the block' do
results = @import.run { |v| v }
- assert_equal results[0], {
- first_name: "Hans",
- last_name: "Müller",
- email: "hans@mueller.com"
- }
- assert_equal results[1], {
- first_name: "Jögi",
- last_name: "Brunz",
- email: "jb@runz.com"
- }
+ assert_equal results, [
+ {
+ first_name: 'Hans',
+ last_name: 'Müller',
+ email: 'hans@mueller.com'
+ },
+ {
+ first_name: 'Jögi',
+ last_name: 'Brunz',
+ email: 'jb@runz.com'
+ }
+ ]
end
end
end
describe '#errors' do
it 'returns an error when a column is missing in the excel' do
- import = UserImport.new("test/files/missing-column.xlsx")
+ import = UserImport.new('test/files/missing-column.xlsx')
assert import.errors[:missing_column].any?
end
it 'returns the model validation errors' do
- import = UserImport.new("test/files/missing-first-name.xlsx").tap(&:run)
+ import = UserImport.new('test/files/missing-first-name.xlsx').tap(&:run)
assert import.errors[:model].any?
assert_equal import.errors[:model], [Excelsior::Error.new(3, ["First name can't be blank"])]
end
@@ -162,7 +166,7 @@ describe Excelsior do
describe '#report' do
describe 'without a block' do
it 'returns the inserted, failed and total number of rows' do
- import = UserImport.new("test/files/missing-first-name.xlsx").tap(&:run)
+ import = UserImport.new('test/files/missing-first-name.xlsx').tap(&:run)
assert_equal 2, import.report.inserted
assert_equal 1, import.report.failed
assert_equal 3, import.report.total
@@ -171,9 +175,9 @@ describe Excelsior do
describe 'with a block' do
it 'returns the inserted, failed and total number of rows' do
- import = UserImport.new("test/files/missing-first-name.xlsx")
+ import = UserImport.new('test/files/missing-first-name.xlsx')
import.run do |v|
- raise "failure!" if v[:first_name].nil?
+ raise 'failure!' if v[:first_name].nil?
v
end
assert_equal 2, import.report.inserted
diff --git a/test/test_helper.rb b/test/test_helper.rb
index 59bb421..ec37b64 100644
--- a/test/test_helper.rb
+++ b/test/test_helper.rb
@@ -1,11 +1,15 @@
-$LOAD_PATH.unshift File.expand_path("../../lib", __FILE__)
-require "excelsior"
+# frozen_string_literal: true
+
+$LOAD_PATH.unshift File.expand_path('../lib', __dir__)
+require 'excelsior'
require 'active_record'
-require "minitest/autorun"
+require 'minitest/autorun'
-class MiniTest::Spec
- before do
- User.delete_all
+module MiniTest
+ class Spec
+ before do
+ User.delete_all
+ end
end
end
--
GitLab