Restrict classes to the gem namespace by saturnflyer · Pull Request #7 · SOFware/sof-cycle

saturnflyer · 2024-07-22T13:50:12Z

By using SOF::Parser this gem was taking ownership of the SOF namespace.
Instead, it should only use the SOF::Cycle namespace in order to prevent conflict with other SOF libraries or applications.

SOF::TimeSpan is outside of the scope of this gem which should be SOF::Cycle and could clash with other
libraries or applications using the SOF namespace.

By using SOF::Parser this gem was taking ownership of the SOF namespace. Instead, it should only use the SOF::Cycle namespace in order to prevent conflict with other SOF libraries or applications.

SOF::TimeSpan is outside of the scope of this gem which should be SOF::Cycle and could clash with other libraries or applications using the SOF namespace.

bkuhlmann

Left some feedback on the first class only but I think some of what I'm talking about could be applied to the TimeSpan class too.

bkuhlmann · 2024-07-22T14:05:49Z

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


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

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

bkuhlmann · 2024-07-22T14:11:40Z

@@ -0,0 +1,108 @@
+# frozen_string_literal: true
+
+require "active_support/core_ext/hash/keys"


💭 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.

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

bkuhlmann · 2024-07-22T14:14:37Z

+        (?<from>F(?<from_date>\d{4}-\d{2}-\d{2}))?$ # optional from
+      /ix
+
+      def self.dormant_capable_kinds = %w[E W]


🎨 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?

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

[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.

bkuhlmann · 2024-07-22T14:23:09Z

+        new(notation_or_parser)
+      end
+
+      def self.load(hash)


🎨 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:

The keys should be a constant.

The defaults should be a constant.

The .load method might not belong on this class? It feels a little out of place, maybe?

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

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.

bkuhlmann · 2024-07-22T14:24:28Z

+      end
+
+      def initialize(notation)
+        @notation = notation&.upcase


🎨 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

bkuhlmann · 2024-07-22T14:29:27Z

+      delegate [:period, :humanized_period] => :time_span
+
+      # Return a TimeSpan object for the period and period_count
+      def time_span


🎨 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.

bkuhlmann · 2024-07-22T14:30:19Z

+
+      def valid? = match.present?
+
+      def inspect = notation


❇️ Yeah, that's nice.

bkuhlmann · 2024-07-22T14:31:43Z

+      def activated_notation(date)
+        return notation unless dormant_capable?
+
+        self.class.load(to_h.merge(from_date: date.to_date)).notation


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

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

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

bkuhlmann · 2024-07-22T14:34:22Z

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


🎨 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?

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

saturnflyer added 2 commits July 22, 2024 09:48

Move the Parser class to a gem namespace

705cf8b

By using SOF::Parser this gem was taking ownership of the SOF namespace. Instead, it should only use the SOF::Cycle namespace in order to prevent conflict with other SOF libraries or applications.

Move TimeSpan to the Cycle namespace

662d16a

SOF::TimeSpan is outside of the scope of this gem which should be SOF::Cycle and could clash with other libraries or applications using the SOF namespace.

bkuhlmann reviewed Jul 22, 2024

View reviewed changes

		@@ -0,0 +1,108 @@
		# frozen_string_literal: true

		require "active_support/core_ext/hash/keys"

Conversation

saturnflyer commented Jul 22, 2024

Uh oh!

bkuhlmann left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bkuhlmann Jul 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bkuhlmann Jul 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bkuhlmann Jul 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

bkuhlmann Jul 22, 2024 •

edited

Loading

bkuhlmann Jul 22, 2024 •

edited

Loading

bkuhlmann Jul 22, 2024 •

edited

Loading