-
Notifications
You must be signed in to change notification settings - Fork 1
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Extract plugins for yarn and ruby to separate responsibilities #87
Conversation
058a4bd
to
5ecaa00
Compare
d214742
to
68e670a
Compare
f4837f3
to
7c9b094
Compare
c53155a
to
c4b8c4f
Compare
fb47fde
to
c3d2548
Compare
@@ -0,0 +1,6 @@ | |||
module CobraCommander | |||
module Ruby | |||
VERSION: String |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Plan on setting types for other files?
def [](key) | ||
Class.new(self) do | ||
define_singleton_method(:key) { key } | ||
define_method(:key) { key } | ||
|
||
def self.inherited(base) | ||
super | ||
superclass.all[key] = base | ||
end | ||
end | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm going to have to open a console in here to unpack what this accomplishes. I wasn't familiar with Forwardable.
What is key
? Is it a name or a path, or what?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I'm not a big fan of how this is right now, but what it achieves is:
I want a class extending the registry to be registered in the registry and have a key attribute used to identify the registered class uniquely within the registry. This key is used to toggle the plugin on/off in the, for instance.
The key is a required class attribute of the source, so I didn't want to just have them define a key, I wanted to initialize it with the inheritance. This is the best I could do.
This defines a class with a key, and once inherited it will register the new class with the given key. The resulting syntax is similar to what AR does with migrations. I wish the code was simpler, but I like the overall solution.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it a category
then perhaps? Or possibly a klass
? --- Maybe even just a registration_key
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work!
This PR splits CobraCommander into three packages:
cobra_commander
: the core models of a cobra app (components and umbrella), the CLI, and tools such as graph, tree, and the executorcobra_commander-ruby
: the source for ruby packages within a cobra appcobra_commander-yarn
: the source for JS/yarn workspaces packages within a cobra appWith this change, a cobra app should now include the core library and the necessary plugins within the
cobra
bundle group.The plugins should gain extra responsibility in the near future, like the introduction of Cobra Generators and dependency management (add/remove)
Todo