From 8344c42313e395acaf8558dacfbcc4a82cc92410 Mon Sep 17 00:00:00 2001 From: Geremia Taglialatela Date: Sun, 17 Sep 2023 17:39:54 +0200 Subject: [PATCH] Replace ternary operators with if...else Ternary operator are bad for coverage, in fact some branches were not tested because of wrong or missing specs. Also, ternary operator on multiple lines are parsed as a single line, so they should be removed too This commit also fixes a spec and adds a missing one for easy use cases --- .rubocop_todo.yml | 19 ++-------- lib/chrono_model/adapter/tsrange.rb | 19 ++++++++-- lib/chrono_model/conversions.rb | 8 ++++- lib/chrono_model/patches/association.rb | 9 +++-- lib/chrono_model/patches/relation.rb | 3 +- lib/chrono_model/time_machine.rb | 22 +++++++++--- .../time_machine/history_model.rb | 9 +++-- lib/chrono_model/time_machine/time_query.rb | 20 +++++++++-- lib/chrono_model/time_machine/timeline.rb | 36 ++++++++++++++----- spec/chrono_model/conversions_spec.rb | 7 ++++ .../chrono_model/time_machine/history_spec.rb | 2 +- 11 files changed, 111 insertions(+), 43 deletions(-) diff --git a/.rubocop_todo.yml b/.rubocop_todo.yml index 6a9151f4..f80b067d 100644 --- a/.rubocop_todo.yml +++ b/.rubocop_todo.yml @@ -13,7 +13,7 @@ Lint/TripleQuotes: # Configuration parameters: AllowedMethods, AllowedPatterns, CountRepeatedAttributes. Metrics/AbcSize: - Max: 72 + Max: 73 # Configuration parameters: CountComments, CountAsOne, AllowedMethods, AllowedPatterns, inherit_mode. # AllowedMethods: refine @@ -26,7 +26,7 @@ Metrics/CyclomaticComplexity: # Configuration parameters: CountComments, CountAsOne, AllowedMethods, AllowedPatterns. Metrics/MethodLength: - Max: 44 + Max: 62 # Configuration parameters: CountComments, CountAsOne. Metrics/ModuleLength: @@ -38,7 +38,7 @@ Metrics/ParameterLists: # Configuration parameters: AllowedMethods, AllowedPatterns. Metrics/PerceivedComplexity: - Max: 22 + Max: 26 # This cop supports unsafe autocorrection (--autocorrect-all). # Configuration parameters: EnforcedStyleForLeadingUnderscores. @@ -62,11 +62,6 @@ Naming/VariableNumber: Exclude: - 'lib/chrono_model/adapter/upgrade.rb' -# This cop supports unsafe autocorrection (--autocorrect-all). -Performance/UnfreezeString: - Exclude: - - 'lib/chrono_model/time_machine/timeline.rb' - # This cop supports safe autocorrection (--autocorrect). # Configuration parameters: EnforceForPrefixed. Rails/Delegate: @@ -131,14 +126,6 @@ Style/MultilineIfModifier: Exclude: - 'lib/chrono_model/adapter.rb' -# This cop supports safe autocorrection (--autocorrect). -Style/MultilineTernaryOperator: - Exclude: - - 'lib/chrono_model/patches/association.rb' - - 'lib/chrono_model/time_machine.rb' - - 'lib/chrono_model/time_machine/history_model.rb' - - 'lib/chrono_model/time_machine/timeline.rb' - # This cop supports safe autocorrection (--autocorrect). # Configuration parameters: AllowMethodComparison, ComparisonsThreshold. Style/MultipleComparison: diff --git a/lib/chrono_model/adapter/tsrange.rb b/lib/chrono_model/adapter/tsrange.rb index 26d1cd06..e005e7fa 100644 --- a/lib/chrono_model/adapter/tsrange.rb +++ b/lib/chrono_model/adapter/tsrange.rb @@ -34,9 +34,24 @@ def cast_value(value) def extract_bounds(value) from, to = value[1..-2].split(',') + + from_bound = + if value[1] == ',' || from == '-infinity' + nil + else + from[1..-2] + end + + to_bound = + if value[-2] == ',' || to == 'infinity' + nil + else + to[1..-2] + end + { - from: value[1] == ',' || from == '-infinity' ? nil : from[1..-2], - to: value[-2] == ',' || to == 'infinity' ? nil : to[1..-2] + from: from_bound, + to: to_bound } end end diff --git a/lib/chrono_model/conversions.rb b/lib/chrono_model/conversions.rb index 45f8e7b1..069edf48 100644 --- a/lib/chrono_model/conversions.rb +++ b/lib/chrono_model/conversions.rb @@ -10,7 +10,13 @@ def string_to_utc_time(string) return string if string.is_a?(Time) if string =~ ISO_DATETIME - usec = $7.nil? ? '000000' : $7.ljust(6, '0') # .1 is .100000, not .000001 + # .1 is .100000, not .000001 + usec = + if $7.nil? + '000000' + else + $7.ljust(6, '0') + end Time.utc $1.to_i, $2.to_i, $3.to_i, $4.to_i, $5.to_i, $6.to_i, usec.to_i end end diff --git a/lib/chrono_model/patches/association.rb b/lib/chrono_model/patches/association.rb index 258047b5..2adc505b 100644 --- a/lib/chrono_model/patches/association.rb +++ b/lib/chrono_model/patches/association.rb @@ -41,9 +41,12 @@ def _chrono_record? end def _chrono_target? - @_target_klass ||= reflection.options[:polymorphic] ? - owner.public_send(reflection.foreign_type).constantize : - reflection.klass + @_target_klass ||= + if reflection.options[:polymorphic] + owner.public_send(reflection.foreign_type).constantize + else + reflection.klass + end @_target_klass.chrono? end diff --git a/lib/chrono_model/patches/relation.rb b/lib/chrono_model/patches/relation.rb index 3d7ffe33..2c149ee8 100644 --- a/lib/chrono_model/patches/relation.rb +++ b/lib/chrono_model/patches/relation.rb @@ -9,7 +9,8 @@ module Relation def preload_associations(records) # :nodoc: preload = preload_values preload += includes_values unless eager_loading? - scope = strict_loading_value ? StrictLoadingScope : nil + scope = StrictLoadingScope if strict_loading_value + preload.each do |associations| ActiveRecord::Associations::Preloader.new( records: records, associations: associations, scope: scope, model: model, as_of_time: as_of_time diff --git a/lib/chrono_model/time_machine.rb b/lib/chrono_model/time_machine.rb index 9fe3efa3..fc531a57 100644 --- a/lib/chrono_model/time_machine.rb +++ b/lib/chrono_model/time_machine.rb @@ -115,8 +115,12 @@ def has_timeline(options) changes = options.delete(:changes) assocs = history.has_timeline(options) - attributes = changes.present? ? - Array.wrap(changes) : assocs.map(&:name) + attributes = + if changes.present? + Array.wrap(changes) + else + assocs.map(&:name) + end attribute_names_for_history_changes.concat(attributes.map(&:to_s)) end @@ -210,7 +214,11 @@ def succ # Returns the current history version # def current_version - self.historical? ? self.class.find(self.id) : self + if self.historical? + self.class.find(self.id) + else + self + end end # Returns the differences between this entry and the previous history one. @@ -231,8 +239,12 @@ def changes_against(ref) old, new = ref.public_send(attr), self.public_send(attr) changes.tap do |c| - changed = old.respond_to?(:history_eql?) ? - !old.history_eql?(new) : old != new + changed = + if old.respond_to?(:history_eql?) + !old.history_eql?(new) + else + old != new + end c[attr] = [old, new] if changed end diff --git a/lib/chrono_model/time_machine/history_model.rb b/lib/chrono_model/time_machine/history_model.rb index baa06d7d..ad5adf9d 100644 --- a/lib/chrono_model/time_machine/history_model.rb +++ b/lib/chrono_model/time_machine/history_model.rb @@ -71,9 +71,12 @@ def as_of(time) end def virtual_table_at(time, table_name: nil) - virtual_name = table_name ? - connection.quote_table_name(table_name) : - superclass.quoted_table_name + virtual_name = + if table_name + connection.quote_table_name(table_name) + else + superclass.quoted_table_name + end "(#{at(time).to_sql}) #{virtual_name}" end diff --git a/lib/chrono_model/time_machine/time_query.rb b/lib/chrono_model/time_machine/time_query.rb index ffe97caa..4d190b7d 100644 --- a/lib/chrono_model/time_machine/time_query.rb +++ b/lib/chrono_model/time_machine/time_query.rb @@ -23,11 +23,21 @@ def time_query_sql(match, time, range, options) "NOT (#{build_time_query_at(time, range)})" when :before - op = options.fetch(:inclusive, true) ? '&&' : '@>' + op = + if options.fetch(:inclusive, true) + '&&' + else + '@>' + end build_time_query(['NULL', time_for_time_query(time, range)], range, op) when :after - op = options.fetch(:inclusive, true) ? '&&' : '@>' + op = + if options.fetch(:inclusive, true) + '&&' + else + '@>' + end build_time_query([time_for_time_query(time, range), 'NULL'], range, op) else @@ -67,7 +77,11 @@ def build_time_query_at(time, range) # If both edges of the range are the same the query fails using the '&&' operator. # The correct solution is to use the <@ operator. - time.first == time.last ? time.first : time + if time.first == time.last + time.first + else + time + end else time_for_time_query(time, range) end diff --git a/lib/chrono_model/time_machine/timeline.rb b/lib/chrono_model/time_machine/timeline.rb index ad680703..2e631936 100644 --- a/lib/chrono_model/time_machine/timeline.rb +++ b/lib/chrono_model/time_machine/timeline.rb @@ -7,10 +7,21 @@ module Timeline # history record exists. Takes temporal associations into account. # def timeline(record = nil, options = {}) - rid = record.respond_to?(:rid) ? record.rid : record.id if record + rid = + if record + if record.respond_to?(:rid) + record.rid + else + record.id + end + end - assocs = options.key?(:with) ? - timeline_associations_from(options[:with]) : timeline_associations + assocs = + if options.key?(:with) + timeline_associations_from(options[:with]) + else + timeline_associations + end models = [] models.push self if self.chrono? @@ -36,8 +47,14 @@ def timeline(record = nil, options = {}) end end - relation = relation - .order("ts #{options[:reverse] ? 'DESC' : 'ASC'}") + relation_order = + if options[:reverse] + 'DESC' + else + 'ASC' + end + + relation = relation.order("ts #{relation_order}") relation = relation.from(%["public".#{quoted_table_name}]) unless self.chrono? relation = relation.where(id: rid) if rid @@ -53,9 +70,12 @@ def timeline(record = nil, options = {}) end if rid && !options[:with] - sql << (self.chrono? ? %{ - AND ts <@ ( SELECT tsrange(min(lower(validity)), max(upper(validity)), '[]') FROM #{quoted_table_name} WHERE id = #{rid} ) - } : %[ AND ts < NOW() ]) + sql << + if self.chrono? + %{ AND ts <@ ( SELECT tsrange(min(lower(validity)), max(upper(validity)), '[]') FROM #{quoted_table_name} WHERE id = #{rid} ) } + else + %[ AND ts < NOW() ] + end end sql << " LIMIT #{options[:limit].to_i}" if options.key?(:limit) diff --git a/spec/chrono_model/conversions_spec.rb b/spec/chrono_model/conversions_spec.rb index c7289efe..5b9e7637 100644 --- a/spec/chrono_model/conversions_spec.rb +++ b/spec/chrono_model/conversions_spec.rb @@ -26,6 +26,13 @@ it { expect(string_to_utc_time.usec).to eq 129_000 } # Ref Issue #32 end + context 'with a valid UTC string without usec' do + let(:string) { '2017-02-06 09:46:31' } + + it { is_expected.to be_a(Time) } + it { expect(string_to_utc_time.usec).to eq 0 } + end + context 'with an invalid UTC time string' do let(:string) { 'foobar' } diff --git a/spec/chrono_model/time_machine/history_spec.rb b/spec/chrono_model/time_machine/history_spec.rb index 63de4b4a..c2510ebe 100644 --- a/spec/chrono_model/time_machine/history_spec.rb +++ b/spec/chrono_model/time_machine/history_spec.rb @@ -109,7 +109,7 @@ end describe 'from #as_of' do - subject { $t.foo.as_of(Time.now) } + subject { $t.foo.as_of(Time.now).current_version } it { is_expected.to eq $t.foo } end