Rip out activesupport #2
Conversation
KnVerey
force-pushed
the
no-activesupport
branch
from
6d7c781
to
8055aa2
Compare
| @@ -293,23 +288,48 @@ class KubernetesDeploy | |||
| end | |||
| end | |||
|
|
|||
| module DescendantsTracker | |||
There was a problem hiding this comment.
I believe these are the only pieces of https://github.com/rails/rails/blob/master/activesupport/lib/active_support/descendants_tracker.rb we actually need.
| end | ||
|
|
||
| def for_type(type, name, namespace, file) | ||
| if subclass = direct_descendants.find { |subclass| subclass.handled_type.downcase == type } |
There was a problem hiding this comment.
If what you need is getting Service.new from "service" as a string, you can do the following:
type = "service"
Object.const_get(type.capitalize)
This way you won't need DescendantsTracker at all.
There was a problem hiding this comment.
True (even KubernetesResource.const_get(type.capitalize), I believe?), but is that really better? It feels like a bit of a cheat (i.e. reliable because by convention constants capitalized "Randomtype" will be classes), and it would prevent us from camelizing the class names (e.g. PersistentVolumeClaim -> Persistentvolumeclaim).
There was a problem hiding this comment.
This is how Ruby developers usually do (iterating over DescendantsTracker sounds like an antipattern in this case).
For PVL, the right thing would be:
irb(main):025:0> "persistent_volume_claim".classify
=> "PersistentVolumeClaim"
(classify comes from AS)
There was a problem hiding this comment.
(assuming persistent_volume_claim comes from the type)
There was a problem hiding this comment.
(assuming persistent_volume_claim comes from the type)
Sadly no. This is coming from kubectl as persistentvolumeclaim.
This is how Ruby developers usually do (iterating over DescendantsTracker sounds like an antipattern in this case). For PVL, the right thing would be: ... (classify comes from AS)
Calling it an antipattern doesn't really help me understand why you think one option is better than the other. Both approaches are available in ActiveSupport. What I want is to find the subclass that exists to support that type, so doing exactly that doesn't sound inherently bad to me.
There was a problem hiding this comment.
I wouldn't say antipattern. But it's definitely better to strive for "strategy pattern" over iterating on all candidates to find what you are looking for.
Not that it makes a huge difference performance wise (since in this kind of cases, the candidates set is generally small), but it usually makes simpler, more direct code to read.
NB: I'm just answering this specific thread cause I saw some keywords that triggered me. But I haven't kept a close watch on that PR, so feel free to disregard my opinion.
|
Closing. The dependency problem was fixed quickly enough that this wasn't necessary. |
# This is the 1st commit message: New tests # This is the commit message #2: Rework restart test
Disable deprecated annotation warning
@kirs @sirupsen @Shopify/byroot
This rips out the dependency on ActiveSupport to get around https://github.com/Shopify/shipit/issues/124. As you can see, not having it is a very small inconvenience.
I did a quick 🎩 of a happy path deploy of my test app.