Skip to content

Commit

Permalink
Replace ternary operators with if...else
Browse files Browse the repository at this point in the history
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
  • Loading branch information
tagliala committed Sep 17, 2023
1 parent 89bf2bc commit 8344c42
Show file tree
Hide file tree
Showing 11 changed files with 111 additions and 43 deletions.
19 changes: 3 additions & 16 deletions .rubocop_todo.yml

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

19 changes: 17 additions & 2 deletions lib/chrono_model/adapter/tsrange.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
8 changes: 7 additions & 1 deletion lib/chrono_model/conversions.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
9 changes: 6 additions & 3 deletions lib/chrono_model/patches/association.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
3 changes: 2 additions & 1 deletion lib/chrono_model/patches/relation.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
22 changes: 17 additions & 5 deletions lib/chrono_model/time_machine.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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.
Expand All @@ -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
Expand Down
9 changes: 6 additions & 3 deletions lib/chrono_model/time_machine/history_model.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
20 changes: 17 additions & 3 deletions lib/chrono_model/time_machine/time_query.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down
36 changes: 28 additions & 8 deletions lib/chrono_model/time_machine/timeline.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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?
Expand All @@ -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
Expand All @@ -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)
Expand Down
7 changes: 7 additions & 0 deletions spec/chrono_model/conversions_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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' }

Expand Down
2 changes: 1 addition & 1 deletion spec/chrono_model/time_machine/history_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down

0 comments on commit 8344c42

Please sign in to comment.