Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Restrict classes to the gem namespace #7

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 0 additions & 2 deletions lib/sof-cycle.rb
Original file line number Diff line number Diff line change
Expand Up @@ -12,5 +12,3 @@
require_relative "sof/cycle"

Dir[File.join(__dir__, "sof", "cycles", "*.rb")].each { |file| require file }

require_relative "sof/time_span"
3 changes: 2 additions & 1 deletion lib/sof/cycle.rb
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
# frozen_string_literal: true

require_relative "parser"
require_relative "cycle/parser"
require_relative "cycle/time_span"

module SOF
class Cycle
Expand Down
108 changes: 108 additions & 0 deletions lib/sof/cycle/parser.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,108 @@
# frozen_string_literal: true

require "active_support/core_ext/hash/keys"
Copy link

@bkuhlmann bkuhlmann Jul 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💭 This is a heavy set of requirements on Rails/ActiveRecord. Would it be possible to decouple this gem from Rails so it's pure Ruby since Rails is so heavyweight for a gem? For example, the Refinements gem is perfect for situations like this.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was extracted from a rails app. Yes, it would be good to limit dependencies but it's not a high priority at the moment

require "active_support/core_ext/object/blank"
require "active_support/core_ext/object/inclusion"
require "active_support/core_ext/hash/reverse_merge"
require "active_support/isolated_execution_state"

module SOF
# This class is not intended to be referenced directly.
# This is an internal implementation of Cycle behavior.
class Cycle
class Parser

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎨 Would it be possible to make this a module instead of a class in order to avoid the Nested Classes antipattern?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would be a good thing, yes. In future changes perhaps

extend Forwardable
PARTS_REGEX = /
^(?<vol>V(?<volume>\d*))? # optional volume
(?<set>(?<kind>L|C|W|E) # kind
(?<period_count>\d+) # period count
(?<period_key>D|W|M|Q|Y)?)? # period_key
(?<from>F(?<from_date>\d{4}-\d{2}-\d{2}))?$ # optional from
/ix

def self.dormant_capable_kinds = %w[E W]

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎨 Would a constant be better than a class method since this is what constants are meant for?

💡 We should freeze this since there is no additional behavior?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should freeze it, yes.
In my opinion, constants are great for internal reference but methods are better for external reference.
I'm looking at this on mobile so I don't actually know where this is used but regardless the scope of this PR is namespace changes so that would need to be addressed in the future if necessary

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[C]onstants are great for internal reference but methods are better for external reference.

Hmm, that's interesting. I've not heard that before. I suppose I could see that making sense if the class method was made private. At least that would strengthen the intent of class method being a internal reference.


def self.for(notation_or_parser)
return notation_or_parser if notation_or_parser.is_a? self

new(notation_or_parser)
end

def self.load(hash)
Copy link

@bkuhlmann bkuhlmann Jul 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎨 Would it make sense to inject dependencies for more flexibility? For example, here's a slight refactoring:

def self.load(keys: %i[volume kind period_count period_key], defaults: {volume: 1}, **attributes)
  updated_attributes = defaults.merge! attributes.symbolize_keys!
  text = "V#{updated_attributes.values_at(*keys).join}"

  return new text unless updated_attributes[:from_date]

  new [text, "F#{updated_attributes[:from_date]}"].join
end

...but even with this refactoring, there are a few issues:

  1. The keys should be a constant.
  2. The defaults should be a constant.
  3. The .load method might not belong on this class? It feels a little out of place, maybe?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, I don't think so. This method is designed to be used in ActiveRecord serialization which depends on a simple dump and load

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, so for serialization then. That would be a good candidate for serialization objects which would clarify the separation of concerns. Can't tackle that now but would definitely clarify the functionality.

hash.symbolize_keys!
hash.reverse_merge!(volume: 1)
keys = %i[volume kind period_count period_key]
str = "V#{hash.values_at(*keys).join}"
return new(str) unless hash[:from_date]

new([str, "F#{hash[:from_date]}"].join)
end

def initialize(notation)
@notation = notation&.upcase
Copy link

@bkuhlmann bkuhlmann Jul 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎨 Could we make our code more confident (channeling Avdi Grimm's Confident Ruby book) by casting? Example:

notation = String notation
@notation = notation.upcase
@match = notation.match PARTS_REGEX

@match = @notation&.match(PARTS_REGEX)
end

attr_reader :match, :notation

delegate [:dormant_capable_kinds] => "self.class"
delegate [:period, :humanized_period] => :time_span

# Return a TimeSpan object for the period and period_count
def time_span

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎨 I'm pretty sure this will cause a Ruby shape variations performance warning. To avoid this, we can use @time_span = nil in the constructor.

💡 Here's what I use in my .bashrc to stay on top of performance warnings in case it helps: export RUBYOPT="-W:deprecated -W:performance --yjit --debug-frozen-string-literal". You only need -W:performance to see the performance warnings but you might want the rest too.

@time_span ||= TimeSpan.for(period_count, period_key)
end

def valid? = match.present?

def inspect = notation

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❇️ Yeah, that's nice.

alias_method :to_s, :inspect

def activated_notation(date)
return notation unless dormant_capable?

self.class.load(to_h.merge(from_date: date.to_date)).notation

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💭 This is interesting. Should we use #with instead of .load? Then you'd be able to avoid self.class.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given that the load method is for serialization, no. This is just reusing it.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, makes more sense now based on your earlier comment.

end

def ==(other) = other.to_h == to_h

def to_h

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎨 If the keys were injected (mentioned earlier in the .load method) then you could zip the keys with the values. ...but I think there's something else going on here because it feels like we have an object wanting be born? By this, I mean, we probably should have a Data object which is injected and answered back. This way this responsibility doesn't fall on this class alone?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Based on the above conversation I now know what the object is that's hidden in this implementation: it's a serializer.

{
volume:,
kind:,
period_count:,
period_key:,
from_date:
}
end

def parses?(notation_id) = kind == notation_id

def active? = !dormant?

def dormant? = dormant_capable? && from_date.nil?

def dormant_capable? = kind.in?(dormant_capable_kinds)

def period_count = match[:period_count]

def period_key = match[:period_key]

def vol = match[:vol] || "V1"

def volume = (match[:volume] || 1).to_i

def from_data
return {} unless from

{from: from}
end

def from_date = match[:from_date]

def from = match[:from]

def kind = match[:kind]
end
end
end
232 changes: 232 additions & 0 deletions lib/sof/cycle/time_span.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,232 @@
# frozen_string_literal: true

module SOF
# This class is not intended to be referenced directly.
# This is an internal implementation of Cycle behavior.
class Cycle
class TimeSpan
extend Forwardable
# TimeSpan objects map Cycle notations to behaviors for their periods
#
# For example:
# 'M' => TimeSpan::DatePeriod::Month
# 'Y' => TimeSpan::DatePeriod::Year
# Read each DatePeriod subclass for more information.
#
class InvalidPeriod < StandardError; end

class << self
# Return a time_span for the given count and period
def for(count, period)
case count.to_i
when 0
TimeSpanNothing
when 1
TimeSpanOne
else
self
end.new(count, period)
end

# Return a notation string from a hash
def notation(hash)
return unless hash.key?(:period)

[
hash.fetch(:period_count) { 1 },
notation_id_from_name(hash[:period])
].compact.join
end

# Return the notation character for the given period name
def notation_id_from_name(name)
type = DatePeriod.types.find do |klass|
klass.period.to_s == name.to_s
end

raise InvalidPeriod, "'#{name}' is not a valid period" unless type

type.code
end
end

# Class used to calculate the windows of time so that
# a TimeSpan object will know the correct end of year,
# quarter, etc.
class DatePeriod
extend Forwardable
class << self
def for(count, period_notation)
@cached_periods ||= {}
@cached_periods[period_notation] ||= {}
@cached_periods[period_notation][count] ||= (for_notation(period_notation) || self).new(count)
@cached_periods[period_notation][count]
end

def for_notation(notation)
types.find do |klass|
klass.code == notation.to_s.upcase
end
end

def types = @types ||= Set.new

def inherited(klass)
DatePeriod.types << klass
end

@period = nil
@code = nil
@interval = nil
attr_reader :period, :code, :interval
end

delegate [:period, :code, :interval] => "self.class"

def initialize(count)
@count = count
end
attr_reader :count

def end_date(date)
@end_date ||= {}
@end_date[date] ||= date + duration
end

def begin_date(date)
@begin_date ||= {}
@begin_date[date] ||= date - duration
end

def duration = count.send(period)

def end_of_period(_) = nil

def humanized_period
return period if count == 1

"#{period}s"
end

class Year < self
@period = :year
@code = "Y"
@interval = "years"

def end_of_period(date)
date.end_of_year
end

def beginning_of_period(date)
date.beginning_of_year
end
end

class Quarter < self
@period = :quarter
@code = "Q"
@interval = "quarters"

def duration
(count * 3).months
end

def end_of_period(date)
date.end_of_quarter
end

def beginning_of_period(date)
date.beginning_of_quarter
end
end

class Month < self
@period = :month
@code = "M"
@interval = "months"

def end_of_period(date)
date.end_of_month
end

def beginning_of_period(date)
date.beginning_of_month
end
end

class Week < self
@period = :week
@code = "W"
@interval = "weeks"

def end_of_period(date)
date.end_of_week
end

def beginning_of_period(date)
date.beginning_of_week
end
end

class Day < self
@period = :day
@code = "D"
@interval = "days"

def end_of_period(date)
date
end

def beginning_of_period(date)
date
end
end
end
private_constant :DatePeriod

def initialize(count, period_id)
@count = Integer(count, exception: false)
@window = DatePeriod.for(period_count, period_id)
end
attr_reader :window

delegate [:end_date, :begin_date] => :window

def end_date_of_period(date)
window.end_of_period(date)
end

def begin_date_of_period(date)
window.beginning_of_period(date)
end

# Integer value for the period count or nil
def period_count
@count
end

delegate [:period, :duration, :interval, :humanized_period] => :window

# Return a date according to the rules of the time_span
def final_date(date)
return unless period

window.end_date(date.to_date)
end

def to_h
{
period:,
period_count:
}
end

class TimeSpanNothing < self
end

class TimeSpanOne < self
def interval = humanized_period
end
end
end
end
Loading