-
Notifications
You must be signed in to change notification settings - Fork 16
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 bootstrap/framework code from dev #5
Conversation
true | ||
end | ||
|
||
def self.statsd_increment(metric, **kwargs) |
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'll probably migrate Statsd
over here too, but for now, these two are interface methods.
|
||
module CLI | ||
module Kit | ||
class BaseCommand |
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.
This is Dev::Command
, more or less. I was able to extract a reasonable amount of it to here and subclass this over there.
|
||
module CLI | ||
module Kit | ||
module CommandRegistry |
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.
Some modest gymnastics here to manage variables on this command vs. access the module it's extended in.
attr_accessor :registry_target | ||
end | ||
|
||
def resolve_contextual_command |
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.
The concept of "Project-local" and "Type" commands was generalized a bit into "Contextual" commands. The interface to add contextual commands to a command registry is:
resolve_contextual_command
contextual_aliases
contextual_command_class
This is a pretty niche concern and I don't really expect to build too many tools other than dev
that use this feature.
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.
So this is only included for stuff from a dev.yml
?
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.
Well it's "included" everywhere depending on what you mean, but, well: https://github.com/Shopify/dev/blob/4eea78118cc0f6fe4180f37aef97ab676101915c/lib/dev/commands.rb#L7-L19
# if ENV['XDG_CONFIG_HOME'] is not set, we default to ~/.config, e.g.: | ||
# ~/.config/tool/config | ||
# | ||
def file |
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.
This is different. I've overridden it back to the current behaviour in dev
, but this is what we probably should have done initially. It's a bit tricky to migrate now, but I might shave that yak eventually.
module CLI | ||
module Kit | ||
class EntryPoint | ||
# Interface methods: You may want to implement these: |
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.
The abstraction feels a little rough here but four miscellaneous subclass/interface methods seems tolerable.
lib/cli/kit/report_errors.rb
Outdated
|
||
# error_reporter should support the interface: | ||
# .call(notify_with, logs = nil, stage = nil, api_key = nil, custom_metadata = {}) | ||
def self.setup(logfile_path, error_reporter) |
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.
Going back and forth on whether to actually bring in bugsnag or leave it at this kind of awkward integration level.
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.
What would an app need to implement to use ReportErrors?
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 made the comment more useful.
attr_accessor :registry_target | ||
end | ||
|
||
def resolve_contextual_command |
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.
So this is only included for stuff from a dev.yml
?
lib/cli/kit/command_registry.rb
Outdated
end | ||
|
||
def self.extended(base) | ||
raise "multiple registries unsupported" if registry_target |
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.
if self.registry_target
to be consistent with the next line?
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.
self
is only needed on the next line because it's a setter, and otherwise it would create a variable called registry_target
and set it to base
. self.
convinces ruby that it's a method call.
lib/cli/kit/config.rb
Outdated
@toolname = toolname | ||
end | ||
|
||
# Fixes a bug where setting a key,val would result in the key growing with whitespace |
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.
We can remove this method and what calls it
|
||
module CLI | ||
module Kit | ||
class Config |
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.
Quick follow up here is to make CLI::Kit::Config
subclass our IniParser (or use it)
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.
Yup. I have bad timing but there wasn't much transformation here so it's an easy follow-up
lib/cli/kit/config.rb
Outdated
end | ||
end | ||
|
||
# Returns a mapping of all srcpath variables in the ~/.devconfig file |
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.
This is not needed outside of dev
lib/cli/kit/config.rb
Outdated
path_config | ||
end | ||
|
||
# Returns a path from config in expanded form |
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.
Not needed outside of dev
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 disagree. I think it's generically useful.
lib/cli/kit/entry_point.rb
Outdated
STDERR.puts(CLI::UI.fmt("{{command:#{EntryPoint.tool_name} #{@command_name}}} was not found")) | ||
end | ||
|
||
# TODO: this can be removed when we drop Sierra support |
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.
Let's just remove this?
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.
We should move OS support to before_initialize
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.
Oh yeah, that was only necessary in the first place when we were trying to get Levenshtein from the stdlib.
lib/cli/kit/report_errors.rb
Outdated
|
||
# error_reporter should support the interface: | ||
# .call(notify_with, logs = nil, stage = nil, api_key = nil, custom_metadata = {}) | ||
def self.setup(logfile_path, error_reporter) |
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.
What would an app need to implement to use ReportErrors?
081f9e7
to
ee270e6
Compare
lib/cli/kit/config.rb
Outdated
@ini.to_s | ||
end | ||
|
||
def un_ini |
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.
This shouldn't be ported. This was my escape hatch that I left in in case of problems.
ee270e6
to
788c46f
Compare
lib/cli/kit/config.rb
Outdated
File.expand_path(File.join(toolname, 'config'), config_home) | ||
end | ||
|
||
def reset |
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.
This probably only makes sense for dev
which doesn't have an instance, or acts like it doesn't.
lib/cli/kit/config.rb
Outdated
return @ini.ini if @ini | ||
@ini = Dev::Helpers::Ini.new(file, default_section: "[global]", convert_types: false) | ||
@ini.parse | ||
if @ini.ini.keys == ["[global]"] |
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.
This if
block and convert_package_cloud
below are specific to the dev
config migration.
788c46f
to
f32f9dd
Compare
lib/cli/kit/config.rb
Outdated
return @ini if @ini | ||
@ini = CLI::Kit::Ini | ||
.new(file, default_section: "[global]", convert_types: false) | ||
.tap(&:parse) |
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.
😍 .tap(&:parse)
. Now @ini ||=
, killing the early return.
e4e3f1e
to
748b5ad
Compare
Ok, I have tests passing on |
This PR extracts a bunch of the really generic bootstrap-level code from
dev
. Much of it is directly moved and renamespaced. I'm adding comments to draw attention to what got a more serious facelift.