From 6958b8f50917271a0a5a54f750044e3b9197c8e8 Mon Sep 17 00:00:00 2001 From: Holger Just Date: Thu, 16 Feb 2017 22:36:41 +0100 Subject: [PATCH] Add Rubocop config and resolve most style issues --- .rubocop.yml | 94 +++++++++++++++++++ Rakefile | 2 +- lib/rackstash/buffer.rb | 8 +- lib/rackstash/fields/abstract_collection.rb | 24 +++-- lib/rackstash/fields/array.rb | 22 ++--- lib/rackstash/fields/hash.rb | 9 +- lib/rackstash/fields/tags.rb | 5 +- lib/rackstash/logger.rb | 5 +- lib/rackstash/message.rb | 4 +- spec/rackstash/buffer_spec.rb | 2 +- .../fields/abstract_collection_spec.rb | 15 ++- spec/rackstash/fields/array_spec.rb | 18 ++-- spec/rackstash/fields/hash_spec.rb | 6 +- spec/rackstash/formatter_spec.rb | 5 +- spec/rackstash/logger_spec.rb | 1 - spec/rackstash_spec.rb | 2 +- 16 files changed, 158 insertions(+), 64 deletions(-) create mode 100644 .rubocop.yml diff --git a/.rubocop.yml b/.rubocop.yml new file mode 100644 index 0000000..039f88b --- /dev/null +++ b/.rubocop.yml @@ -0,0 +1,94 @@ +AllCops: + TargetRubyVersion: 2.1 + +# Sometimes we want to capture all exceptions. We need to make sure to re-raise +# these again in any case. +Lint/RescueException: + Enabled: false + +# As long as methods and classes are readable, this should be fine. +# In any case, this shouldn't be a hard error. We try to improve where sensible +# using tools like RubyCritic instead +Metrics/AbcSize: + Enabled: false +Metrics/CyclomaticComplexity: + Enabled: false +Metrics/PerceivedComplexity: + Enabled: false + +# Long classes and methods are not a problem per se, as long as it is kept kind +# of sane. In any case, this shouldn't be a hard error.. +Metrics/ClassLength: + Enabled: false +Metrics/MethodLength: + Enabled: false + +# As long as methods are readable, this should be fine. +# In any case, this shouldn't be a hard error. We try to improve where sensible +# using tools like RubyCritic instead + +# rspec uses long blocks by definition +Metrics/BlockLength: + Exclude: + - 'spec/**/*' + +# Try to keep lines below 80 chars wherever possible though +Metrics/LineLength: + AllowURI: true + Max: 100 + +Style/BlockDelimiters: + EnforcedStyle: semantic + Exclude: + - 'spec/**/*' + +Style/Copyright: + Enabled: true + Notice: '^# Copyright (\(c\) )?2[0-9]{3} .+' + +# Use === when it makes sense to do so +Style/CaseEquality: + Enabled: false + +# Double negation is fine to reliably produce a Boolean from any value. +Style/DoubleNegation: + Enabled: false + +# Multiline indentation is always tricky. Format it as deemed useful in the +# local method, but try to avoid it at all wherever possible +Style/MultilineOperationIndentation: + Enabled: false + +# We generally use `obj == nil` instead of `obj.nil?` because the former is +# faster. +Style/NilComparison: + Enabled: false + +Style/PercentLiteralDelimiters: + PreferredDelimiters: + '%w': '[]' + '%i': '[]' + +# Sometimes, an explicit self really adds some clarity to the code +# We make sure to not use it when not needed though +Style/RedundantSelf: + Enabled: false + +# Doesn't work with Refinements and is sometimes clearer with the expanded +# version +Style/SymbolProc: + Enabled: false + +Style/RegexpLiteral: + Exclude: + - 'spec/**/*' + +# We know what we are doing and only use the rescue modifier when we really +# don't care +Style/RescueModifier: + Enabled: false + +# Use %w when it makes sense. Use literal arrays where it is more clear. +Style/WordArray: + Enabled: false + diff --git a/Rakefile b/Rakefile index e0fd2ee..3f4614b 100644 --- a/Rakefile +++ b/Rakefile @@ -12,4 +12,4 @@ require 'yard' YARD::Rake::YardocTask.new task doc: :yard -task :default => :spec +task default: :spec diff --git a/lib/rackstash/buffer.rb b/lib/rackstash/buffer.rb index 6f6be07..b558168 100644 --- a/lib/rackstash/buffer.rb +++ b/lib/rackstash/buffer.rb @@ -144,10 +144,10 @@ module Rackstash # @return [self,nil] returns `self` if the buffer was flushed, `nil` # otherwise def flush - if pending? - @sink.flush(self) - self - end + return unless pending? + + @sink.flush(self) + self end # Return all logged messages on the current buffer. diff --git a/lib/rackstash/fields/abstract_collection.rb b/lib/rackstash/fields/abstract_collection.rb index 78afda0..d10120e 100644 --- a/lib/rackstash/fields/abstract_collection.rb +++ b/lib/rackstash/fields/abstract_collection.rb @@ -12,15 +12,15 @@ require 'concurrent' module Rackstash module Fields class AbstractCollection - # Equality — Two collections are equal if they are of exactly the same class - # and contain the same raw data according to `Object#==`. + # Equality -- Two collections are equal if they are of exactly the same + # class and contain the same raw data according to `Object#==`. # # @return [Boolean] `true` if `other` is an object of the same class and # contains the same raw data as this object. def ==(other) self.class == other.class && raw == other.raw end - alias :eql? :== + alias eql? == # Show a human-readable representation of `self`. To get a machine- # readable "export" of the contained data, use {#as_json} or one of its @@ -28,7 +28,7 @@ module Rackstash # # @return [String] human-redable details about the object. def inspect - "#<#{self.class}:#{format '0x%014x', object_id << 1} #{to_s}>" + "#<#{self.class}:#{format '0x%014x', object_id << 1} #{self}>" end # Provide a copy of the wrapped {#raw} data in a format allowing direct @@ -106,17 +106,21 @@ module Rackstash hash = value.each_with_object(Concurrent::Hash.new) do |(k, v), memo| memo[utf8_encode(k)] = normalize(v, scope: scope) end - hash = Rackstash::Fields::Hash.new.tap do |hash_field| - hash_field.raw = hash - end if wrap + if wrap + hash = Rackstash::Fields::Hash.new.tap do |hash_field| + hash_field.raw = hash + end + end return hash when ::Array, ::Set, ::Enumerator array = value.each_with_object(Concurrent::Array.new) do |e, memo| memo << normalize(e, scope: scope) end - array = Rackstash::Fields::Array.new.tap do |array_field| - array_field.raw = array - end if wrap + if wrap + array = Rackstash::Fields::Array.new.tap do |array_field| + array_field.raw = array + end + end return array when ::Time return value.utc.iso8601(ISO8601_PRECISION).freeze diff --git a/lib/rackstash/fields/array.rb b/lib/rackstash/fields/array.rb index 8089385..7704d7f 100644 --- a/lib/rackstash/fields/array.rb +++ b/lib/rackstash/fields/array.rb @@ -13,7 +13,7 @@ module Rackstash end # @!method +(array) - # Concatenation — Returns a new {Rackstash::Fields::Array} built by + # Concatenation -- Returns a new {Rackstash::Fields::Array} built by # concatenating `self` and the given `array` together to produce a # third array. # @@ -21,7 +21,7 @@ module Rackstash # @return [Rackstash::Fields::Array] # @!method -(array) - # Array Difference — Returns a new {Rackstash::Fields::Array} that is a + # Array Difference -- Returns a new {Rackstash::Fields::Array} that is a # copy of `self`, removing any items that also appear in the given # `array`. The order is preserved from `self`. # @@ -29,7 +29,7 @@ module Rackstash # @return [Rackstash::Fields::Array] # @!method |(array) - # Set Union — Returns a new {Rackstash::Fields::Array} by joining `self` + # Set Union -- Returns a new {Rackstash::Fields::Array} by joining `self` # with the given `array`, excluding any duplicates and preserving the # order from `self`. # @@ -37,7 +37,7 @@ module Rackstash # @return [Rackstash::Fields::Array] # @!method &(array) - # Set Intersection — Returns a new {Rackstash::Fields::Array} containing + # Set Intersection -- Returns a new {Rackstash::Fields::Array} containing # elements common to `self` and the given `array`, excluding any # duplicates. The order is preserved from `self`. # @@ -45,7 +45,7 @@ module Rackstash # @return [Rackstash::Fields::Array] %i[+ - | &].each do |op| - class_eval <<-RUBY, __FILE__ , __LINE__ + 1 + class_eval <<-RUBY, __FILE__, __LINE__ + 1 def #{op}(array) new(@raw #{op} normalize(array, wrap: false)) end @@ -92,8 +92,8 @@ module Rackstash value.is_a?(AbstractCollection) ? value.as_json : value } end - alias :to_ary :as_json - alias :to_a :as_json + alias to_ary as_json + alias to_a as_json # Removes all elements from `self`. # @@ -127,7 +127,7 @@ module Rackstash @raw.length end - # Set Union — Add value from `array` to `self` excluding any duplicates + # Set Union -- Add value from `array` to `self` excluding any duplicates # and preserving the order from `self`. # # @param array [Array, ::Array, Proc] an array of values. Each value is @@ -137,12 +137,13 @@ module Rackstash # object (when given). # @return [self] # - # @see #merge + # @see #| + # @see #merge! def merge(array, scope: nil) new(@raw | normalize(array, wrap: false, scope: scope)) end - # Set Union — Add value from `array` to `self` excluding any duplicates + # Set Union -- Add value from `array` to `self` excluding any duplicates # and preserving the order from `self`. # # @param array [Array, ::Array, Proc] an array of values. Each value is @@ -170,7 +171,6 @@ module Rackstash array.raw = raw end end - end def self.Array(array) diff --git a/lib/rackstash/fields/hash.rb b/lib/rackstash/fields/hash.rb index c322b11..cde69ab 100644 --- a/lib/rackstash/fields/hash.rb +++ b/lib/rackstash/fields/hash.rb @@ -50,7 +50,7 @@ module Rackstash @raw[key] = normalize(value) end - alias :store :[]= + alias store []= # @return [::Hash] deep-transforms the hash into a plain Ruby Hash def as_json(*) @@ -59,8 +59,8 @@ module Rackstash memo[key] = value end end - alias :to_hash :as_json - alias :to_h :as_json + alias to_hash as_json + alias to_h as_json # Removes all key-value pairs from `self`. # @@ -143,6 +143,7 @@ module Rackstash # @param scope [Object, nil] if `hash` or any of its (deeply-nested) # values is a proc, it will be called in the instance scope of this # object (when given). + # # @yield [key, old_val, new-val] if a block is given and there is a # duplicate key, we call the block and use its return value as the value # to insert @@ -179,7 +180,7 @@ module Rackstash end self end - alias :update :merge! + alias update merge! # @param key [String] The name of a key to check. This MUST be a correctly # encoded String in order to return valid results diff --git a/lib/rackstash/fields/tags.rb b/lib/rackstash/fields/tags.rb index 8e0e9af..10cb1c1 100644 --- a/lib/rackstash/fields/tags.rb +++ b/lib/rackstash/fields/tags.rb @@ -24,8 +24,8 @@ module Rackstash def as_json(*) @raw.to_a end - alias :to_ary :as_json - alias :to_a :as_json + alias to_ary as_json + alias to_a as_json def clear @raw.clear @@ -83,4 +83,3 @@ module Rackstash end end end - diff --git a/lib/rackstash/logger.rb b/lib/rackstash/logger.rb index c016743..fd81d54 100644 --- a/lib/rackstash/logger.rb +++ b/lib/rackstash/logger.rb @@ -65,12 +65,11 @@ module Rackstash def <<(msg) buffer.add_message Message.new( msg, - time: Time.now.utc.freeze, + time: Time.now.utc.freeze ) msg end - # Set the base log level as either one of the {SEVERITIES} or a # String/Symbol describing the level. When logging a message, it will only # be added if its log level is at or above the base level defined here @@ -262,7 +261,7 @@ module Rackstash msg end - alias :log :add + alias log add # Create a new buffering {Buffer} and puts in on the {BufferStack} for the # current Thread. For the duration of the block, all new logged messages diff --git a/lib/rackstash/message.rb b/lib/rackstash/message.rb index b300a4d..07702a2 100644 --- a/lib/rackstash/message.rb +++ b/lib/rackstash/message.rb @@ -52,9 +52,9 @@ module Rackstash cleanup @formatter.call(severity_label, @time, @progname, @message) end - alias :as_json :to_s + alias as_json to_s # Messages are implicitly conversible to Strings - alias :to_str :to_s + alias to_str to_s def to_json as_json.to_json diff --git a/spec/rackstash/buffer_spec.rb b/spec/rackstash/buffer_spec.rb index a745f83..6994f23 100644 --- a/spec/rackstash/buffer_spec.rb +++ b/spec/rackstash/buffer_spec.rb @@ -21,7 +21,7 @@ describe Rackstash::Buffer do describe '#add_message' do it 'adds a message to the buffer' do msg = double(message: 'Hello World', time: Time.now) - expect(buffer.add_message msg).to equal msg + expect(buffer.add_message(msg)).to equal msg expect(buffer.messages).to eql [msg] end diff --git a/spec/rackstash/fields/abstract_collection_spec.rb b/spec/rackstash/fields/abstract_collection_spec.rb index 93786ca..2331db0 100644 --- a/spec/rackstash/fields/abstract_collection_spec.rb +++ b/spec/rackstash/fields/abstract_collection_spec.rb @@ -191,7 +191,7 @@ describe Rackstash::Fields::AbstractCollection do hash = { 1 => 1, :two => 2, 'three' => 3, nil => 4 } expect(normalize(hash, wrap: false)).to eql( - { '1' => 1, 'two' => 2, 'three' => 3, '' => 4 } + '1' => 1, 'two' => 2, 'three' => 3, '' => 4 ) expect(normalize(hash, wrap: false).keys).to all be_frozen end @@ -316,18 +316,17 @@ describe Rackstash::Fields::AbstractCollection do end it 'resolves nested procs' do - expect(normalize(-> { [-> { 'foo' } ] })).to be_instance_of Rackstash::Fields::Array - expect(normalize(-> { [-> { 'foo' } ] })).to contain_exactly 'foo' + expect(normalize(-> { [-> { 'foo' }] })).to be_instance_of Rackstash::Fields::Array + expect(normalize(-> { [-> { 'foo' }] })).to contain_exactly 'foo' end - it 'returns a raw array returned from a proc with wrap: false' do - expect(normalize(-> { ['foo'] }, wrap: false )).to be_a ::Array + expect(normalize(-> { ['foo'] }, wrap: false)).to be_a ::Array expect(normalize(-> { ['foo'] }, wrap: false)).to eql ['foo'] end it 'returns a raw array returned from a nested proc with wrap: false' do - expect(normalize(-> { [-> { 'foo' }] }, wrap: false )).to be_a ::Array + expect(normalize(-> { [-> { 'foo' }] }, wrap: false)).to be_a ::Array expect(normalize(-> { [-> { 'foo' }] }, wrap: false)).to eql ['foo'] end end @@ -458,7 +457,7 @@ describe Rackstash::Fields::AbstractCollection do inner = proc { :return } outer = proc { inner } - expect(normalize(outer)).to match %r{\A#\z} + expect(normalize(outer)).to match %r{\A#\z} end end @@ -487,7 +486,7 @@ describe Rackstash::Fields::AbstractCollection do obj = double("#{method} - successful") methods[0..i].each_with_index do |check, j| - expect(obj).to receive(:respond_to?).with(check).and_return(i==j) + expect(obj).to receive(:respond_to?).with(check).and_return(i == j) .ordered.once end diff --git a/spec/rackstash/fields/array_spec.rb b/spec/rackstash/fields/array_spec.rb index 78d37a4..330aced 100644 --- a/spec/rackstash/fields/array_spec.rb +++ b/spec/rackstash/fields/array_spec.rb @@ -194,12 +194,12 @@ describe Rackstash::Fields::Array do end it 'resolves nested procs' do - expect(array.concat(-> { [-> { :foo } ] } )).to contain_exactly 'foo' + expect(array.concat(-> { [-> { :foo }] })).to contain_exactly 'foo' end it 'resolves nested procs with a custom scope' do expect( - array.concat(-> { [self, -> { self.to_s.upcase } ] }, scope: :stuff) + array.concat(-> { [self, -> { self.to_s.upcase }] }, scope: :stuff) ).to contain_exactly 'stuff', 'STUFF' end end @@ -229,19 +229,19 @@ describe Rackstash::Fields::Array do describe '#merge' do it 'returns the union of elements' do array[0] = 'existing' - expect(array.merge ['new', :existing, -> { 123 }]) + expect(array.merge(['new', :existing, -> { 123 }])) .to contain_exactly('existing', 'new', 123) .and be_a(Rackstash::Fields::Array) end it 'returns a new Array' do - expect(array.merge [:foo]).to be_a(Rackstash::Fields::Array) - expect(array.merge [:foo]).not_to equal array + expect(array.merge([:foo])).to be_a(Rackstash::Fields::Array) + expect(array.merge([:foo])).not_to equal array end it 'resolves nested procs with a custom scope' do expect( - array.merge(-> { [self, -> { self.to_s.upcase } ] }, scope: :stuff) + array.merge(-> { [self, -> { self.to_s.upcase }] }, scope: :stuff) ).to contain_exactly 'stuff', 'STUFF' end end @@ -249,17 +249,17 @@ describe Rackstash::Fields::Array do describe '#merge!' do it 'sets the union of elements to self' do array[0] = 'existing' - expect(array.merge! ['new', :existing, -> { 123 }]) + expect(array.merge!(['new', :existing, -> { 123 }])) .to contain_exactly 'existing', 'new', 123 end it 'returns self' do - expect(array.merge! [:foo]).to equal array + expect(array.merge!([:foo])).to equal array end it 'resolves nested procs with a custom scope' do expect( - array.merge!(-> { [self, -> { self.to_s.upcase } ] }, scope: :stuff) + array.merge!(-> { [self, -> { self.to_s.upcase }] }, scope: :stuff) ).to contain_exactly 'stuff', 'STUFF' end end diff --git a/spec/rackstash/fields/hash_spec.rb b/spec/rackstash/fields/hash_spec.rb index c13b1d9..270f106 100644 --- a/spec/rackstash/fields/hash_spec.rb +++ b/spec/rackstash/fields/hash_spec.rb @@ -177,7 +177,6 @@ describe Rackstash::Fields::Hash do expect(hash.forbidden_key?('forbidden')).to be true expect(hash.forbidden_key?('foo')).to be false end - end describe '#keys' do @@ -215,7 +214,7 @@ describe Rackstash::Fields::Hash do end it 'merges a normalized hash' do - to_merge = {foo: :bar} + to_merge = { foo: :bar } expect(hash).to receive(:normalize).with(to_merge, anything).ordered.and_call_original expect(hash).to receive(:normalize).with(:bar, anything).ordered.and_call_original @@ -377,7 +376,8 @@ describe Rackstash::Fields::Hash do it 'can specify forbidden_keys' do raw = { foo: :bar, forbidden: 'ignored' } - expect { Rackstash::Fields::Hash(raw, forbidden_fields: ['forbidden']) }.to raise_error ArgumentError + expect { Rackstash::Fields::Hash(raw, forbidden_fields: ['forbidden']) } + .to raise_error ArgumentError end end end diff --git a/spec/rackstash/formatter_spec.rb b/spec/rackstash/formatter_spec.rb index eb050f7..ea7e0bf 100644 --- a/spec/rackstash/formatter_spec.rb +++ b/spec/rackstash/formatter_spec.rb @@ -19,8 +19,8 @@ describe Rackstash::Formatter do expect(formatter.call('ERROR', Time.now, 'progname', 123)).to eql "123\n" end - it 'formats Hashes' do - expect(formatter.call('ERROR', Time.now, 'progname', { k: 'v' })).to eql "{:k=>\"v\"}\n" + it 'formats Arrays' do + expect(formatter.call('ERROR', Time.now, 'progname', [1, 'y'])).to eql "[1, \"y\"]\n" end it 'formats exceptions' do @@ -61,5 +61,4 @@ describe Rackstash::RawFormatter do expect(obj).to receive(:inspect).and_return('object') expect(formatter.call('ERROR', Time.now, 'progname', obj)).to eql 'object' end - end diff --git a/spec/rackstash/logger_spec.rb b/spec/rackstash/logger_spec.rb index 1df72c4..e0e9ddb 100644 --- a/spec/rackstash/logger_spec.rb +++ b/spec/rackstash/logger_spec.rb @@ -378,7 +378,6 @@ describe Rackstash::Logger do expect(logger.fields['key']).to eql 'outer' end - it 'buffers multiple messages' do expect(logger.sink).to receive(:flush).once diff --git a/spec/rackstash_spec.rb b/spec/rackstash_spec.rb index e088e7c..81cd85f 100644 --- a/spec/rackstash_spec.rb +++ b/spec/rackstash_spec.rb @@ -12,7 +12,7 @@ describe Rackstash do end it 'defines SEVERITIES constants' do - expect(Rackstash::SEVERITIES).to eql (0..5).to_a + expect(Rackstash::SEVERITIES).to eql((0..5).to_a) expect(Rackstash::DEBUG).to eql 0 expect(Rackstash::INFO).to eql 1