Skip to content
Open
Show file tree
Hide file tree
Changes from 1 commit
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: 1 addition & 1 deletion lib/sof/cycle.rb
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
# frozen_string_literal: true

require_relative "parser"
require_relative "cycle/parser"

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"

@bkuhlmann bkuhlmann Jul 22, 2024

Copy link
Copy Markdown

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
Copy Markdown
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

Copy link
Copy Markdown

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
Copy Markdown
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]

Copy link
Copy Markdown

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
Copy Markdown
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

Copy link
Copy Markdown

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)

@bkuhlmann bkuhlmann Jul 22, 2024

Copy link
Copy Markdown

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
Copy Markdown
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

Copy link
Copy Markdown

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

@bkuhlmann bkuhlmann Jul 22, 2024

Copy link
Copy Markdown

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

Copy link
Copy Markdown

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

Copy link
Copy Markdown

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

Copy link
Copy Markdown

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
Copy Markdown
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.

Copy link
Copy Markdown

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

Copy link
Copy Markdown

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?

Copy link
Copy Markdown

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
107 changes: 0 additions & 107 deletions lib/sof/parser.rb

This file was deleted.

2 changes: 1 addition & 1 deletion spec/sof/parser_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
require "spec_helper"

module SOF
RSpec.describe Parser, type: :value do
RSpec.describe Cycle::Parser, type: :value do
describe ".load(hash)" do
it "returns a Parser instance" do
hash = {
Expand Down