Zdrve/config #188
Zdrve/config #188
Conversation
| def initialize(config_from = Kennel::Config.from_env, &block) | ||
| @config = Kennel::Config.new(config_from, &block) |
There was a problem hiding this comment.
passing a config instance to config.new seems strange, can we pass in a config instead ?
| def initialize(config_from = Kennel::Config.from_env, &block) | |
| @config = Kennel::Config.new(config_from, &block) | |
| def initialize(config = Kennel::Config.from_env) | |
| @config = config |
There was a problem hiding this comment.
You can pass in anything which responds to to_h.
There was a problem hiding this comment.
if someone needs to configure the config they can do it outside, and we can remove the block interface for initialize
can even make config a keyword-arg to leave room for future additions
There was a problem hiding this comment.
Yielding a mutable config object during build is a very common style that I'd like to keep.
There was a problem hiding this comment.
The most common for I see is keyword-args (also easy to build/debug etc), if things get more complicated then yielding+config object might make sense though.
Kennel is for users that don't necessarily know a lot of ruby, so keeping things simple ("pass in a hash" vs "yield this config object") is nice.
| # A block can be passed to `.new` or `.from_env`. If a block is given, | ||
| # then after any settings have been copied from `from` (if given), | ||
| # a mutable config object will be yielded to the block, so that | ||
| # the config can be set up: | ||
| # | ||
| # config = Kennel::Config.new do |c| | ||
| # c.strict_imports = false | ||
| # end |
There was a problem hiding this comment.
if someone wants to modify the config they can create their own object and pass it in ?
There was a problem hiding this comment.
I meant: we can remove the block interface and do Config.new strict_imports: false to have less api surface makes the input easier to test since we can have a "base" hash that gets modified each test instead of having to deal with a block
| # c.strict_imports = false | ||
| # end | ||
| # | ||
| # Once built, a config object is immutable. |
There was a problem hiding this comment.
not a fan of that, makes testing/experimenting harder 🤷
There was a problem hiding this comment.
I have the exact opposite viewpoint. Mutability makes testing harder. Don't like the config? Make a new one.
There was a problem hiding this comment.
if it's a global object then yeah, if it's per-instance then I don't really see the benefit since there would be a new one per test anyways
(no big deal either way)
| ].freeze | ||
| private_constant :ATTRS | ||
|
|
||
| attr_reader(*ATTRS) |
There was a problem hiding this comment.
No thanks. No validation.
Hardest to move, and least value in doing so.
| @@ -114,7 +120,7 @@ def write_file_if_necessary(path, content) | |||
| end | |||
|
|
|||
| def syncer | |||
| @syncer ||= Syncer.new(api, generated, project_filter: project_filter, tracking_id_filter: tracking_id_filter) | |||
| @syncer ||= Syncer.new(config, api, generated, project_filter: project_filter, tracking_id_filter: tracking_id_filter) | |||
There was a problem hiding this comment.
I like the config as first argument, ideally tracking_id_filter + project_filter would also live in there, but that can come later
| @@ -4,8 +4,11 @@ module Kennel | |||
| module Compatibility | |||
| def self.included(into) | |||
| class << into | |||
| %I[out out= err err= strict_imports strict_imports= generate plan update].each do |sym| | |||
| define_method(sym) { |*args| instance.public_send(sym, *args) } | |||
| %I[generate plan update].each do |sym| | |||
There was a problem hiding this comment.
FYI actually prefer the simpler:
| %I[generate plan update].each do |sym| | |
| [:generate, :plan, :update].each do |sym| |
| config = Kennel::Config.from_env do |c| | ||
| c.strict_imports = false | ||
| end |
There was a problem hiding this comment.
another neat api could be:
| config = Kennel::Config.from_env do |c| | |
| c.strict_imports = false | |
| end | |
| config = Kennel::Config.from_env.merge(strict_imports: false) |
No description provided.