Restrict classes to the gem namespace #7
Conversation
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.
| # This class is not intended to be referenced directly. | ||
| # This is an internal implementation of Cycle behavior. | ||
| class Cycle | ||
| class Parser |
There was a problem hiding this comment.
🎨 Would it be possible to make this a module instead of a class in order to avoid the Nested Classes antipattern?
There was a problem hiding this comment.
That would be a good thing, yes. In future changes perhaps
| @@ -0,0 +1,108 @@ | |||
| # frozen_string_literal: true | |||
|
|
|||
| require "active_support/core_ext/hash/keys" | |||
There was a problem hiding this comment.
💭 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.
There was a problem hiding this comment.
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
| (?<from>F(?<from_date>\d{4}-\d{2}-\d{2}))?$ # optional from | ||
| /ix | ||
|
|
||
| def self.dormant_capable_kinds = %w[E W] |
There was a problem hiding this comment.
🎨 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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
[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.
| new(notation_or_parser) | ||
| end | ||
|
|
||
| def self.load(hash) |
There was a problem hiding this comment.
🎨 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
keysshould be a constant. - The
defaultsshould be a constant. - The
.loadmethod might not belong on this class? It feels a little out of place, maybe?
There was a problem hiding this comment.
No, I don't think so. This method is designed to be used in ActiveRecord serialization which depends on a simple dump and load
There was a problem hiding this comment.
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.
| end | ||
|
|
||
| def initialize(notation) | ||
| @notation = notation&.upcase |
There was a problem hiding this comment.
🎨 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| delegate [:period, :humanized_period] => :time_span | ||
|
|
||
| # Return a TimeSpan object for the period and period_count | ||
| def time_span |
There was a problem hiding this comment.
🎨 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.
|
|
||
| def valid? = match.present? | ||
|
|
||
| def inspect = notation |
| def activated_notation(date) | ||
| return notation unless dormant_capable? | ||
|
|
||
| self.class.load(to_h.merge(from_date: date.to_date)).notation |
There was a problem hiding this comment.
💭 This is interesting. Should we use #with instead of .load? Then you'd be able to avoid self.class.
There was a problem hiding this comment.
Given that the load method is for serialization, no. This is just reusing it.
There was a problem hiding this comment.
Yeah, makes more sense now based on your earlier comment.
|
|
||
| def ==(other) = other.to_h == to_h | ||
|
|
||
| def to_h |
There was a problem hiding this comment.
🎨 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?
There was a problem hiding this comment.
Based on the above conversation I now know what the object is that's hidden in this implementation: it's a serializer.
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.