Gitlab/StrongMemoize: Remove forward declaration of `strong_attribute_attr`
Problem
In https://docs.gitlab.com/ee/development/utilities.html#strongmemoize we suggest two form of declaring strong_memoize_attr
:
-
1️⃣ "post" declaration -
2️⃣ "forward" declaration
class Find
include Gitlab::Utils::StrongMemoize
# :one:
def result
search
end
strong_memoize_attr :result
# :two:
strong_memoize_attr :enabled?, :enabled
def enabled?
Feature.enabled?(:some_feature)
end
end
private
and protected
:
class Foo
def foo
end
private :foo
end
However,
private :foo
def foo
end
__END__
Traceback (most recent call last):
1: from foo.rb:1:in `<main>'
foo.rb:1:in `private': undefined method `foo' for class `Object' (NameError)
For this reason it's surprising to see that strong_memoize_attr
supports this style.
Proposed solution
Remove the ability to declare strong_memoize_attr
before the method definition.
This following the code should raise NameError
similar to what access modifiers are doing:
# :two:
strong_memoize_attr :enabled?, :enabled
def enabled?
Feature.enabled?(:some_feature)
end
"Inline" declaration
In theory the following code will also work:
strong_memoize_attr def foo
end
But it seems this is not something we prefer in the codebase.
Discussion
The following discussion from !102915 (merged) should be addressed:
-
@splattael started a discussion: (+1 comment) Related to !102915 (comment 1165797188)
Opened a new thread to have more room to discuss.
I only now saw that we allow defining
strong_memoize_attr
before the actual method definition. That's neat but also a bit sneaky🤓 https://gitlab.com/gitlab-org/gitlab/-/blob/master/spec/lib/gitlab/utils/strong_memoize_spec.rb#L38
Although it works I am wondering if queuing is actually needed because it's surprising and does not match the behavior of other modifiers like
private
andprotected
.private :foo def foo end __END__ Traceback (most recent call last): 1: from foo.rb:1:in `<main>' foo.rb:1:in `private': undefined method `foo' for class `Object' (NameError)
Suggestion (non-blocking) Thoughts on removing this feature from
Gitlab/StrongMemoize
?I'll create a follow-up.