Add Config class for debug, open_timeout, and idle_response_timeout
#291
Conversation
nevans
changed the title
debug, open_timeout, and idle_response_timeout
nevans
force-pushed
the
config-class
branch
2 times, most recently
from
a769c25
to
926df8c
Compare
Nothing uses this new Config class yet. Currently, it holds only three config options: `debug`, `open_timeout`, and `idle_response_timeout`, but more will be added soon. This version does not have any support for inheritance. That will be added in one of the next commits.
Config values are stored in a struct rather than ivars to simplify: * ensuring that all config objects share a single object shape * querying only locally configured values, e.g for inspection.
Inheritance forms a singly-linked-list, so lookup will be O(n) on the
number of ancestors. So arbitrarily deep ancestor lists may have poor
performance, but normal cases should be fine. Without customization,
ancestor trees will be three or four deep:
client -> [versioned ->] global -> default
Config.global inherits from Config.default
This is implemented as another module included under the other two, still based on overriding `attr_accessor`. Types are only enforced by the attr_writer methods. Fortunately, rdoc isn't confused by keyword arguments to `attr_accessor`, so the type can be added to that. Currently, only `:boolean` and `Integer` are supported, but it should be easy to add more.
An (unused) client config object has been added to both Net::IMAP and to ResponseParser. Every client creates its own config object. By default, the client config inherits from the global config. Unlike the client, which always creates a new Config object, the parser simply shares the client's config.
In Net::IMAP, the `@@debug` cvar is replaced by `config.debug?`. And in ResponseParser, references to the global `Net::IMAP.debug` is replaced by `config.debug?`. By default, the client's own config won't have a value set for `debug`, so it will still inherit from the global `Net::IMAP.config.debug?`. Since the parser "belongs" to a client, it uses the client's configuration rather than simply following the global debug mode.
|
@shugo any objections to this? Honestly, it feels a bit overengineered to me, for just three variables with global defaults and client overrides. But, I'm hoping the inheritance flexibility will pay itself off as more config options are added. I've updated #97 (with #293) and added #294, based on this config implementation. The only remaining piece to complete #288 is a Also, this is the last significant feature I want for the 0.4 branch. With this in place, we can choose to backport deprecation warnings, with or without any new feature or behavior. For example, I want the default config in v0.6 to disallow all implicit use of |
|
@nevans I'm not against adding |
Good question. Config will always be loaded. 😆 I switched it to use |
|
@shugo I did switch it to |
I accidentally merged #291 without pushing this update first! Oops. Co-authored-by: Shugo Maeda <shugo@ruby-lang.org>
|
Pushed the autoload/require_relative switch in #295. |
This enables:
Net::IMAP.debugopen_timeoutandidle_response_timeoutThis feels like overkill for just these three config values, but it's written in the context of dozens of other future config options as indicated by #288. And this should make adding new config options as simple as adding new
attr_accessor(and default values) to the Config class. Regarding the rest of the proposal in #288, this implements the basic config inheritance described there. Note that the backward compatibilityload_defaultsmentioned by #288 has not been implemented for this PR.Two other significant implementation details, not originally mentioned in the #288 proposal:
Integer(input), so I preserved that.attr_accessor, which was my original plan, this PR delegates to an internalStruct. This is partly a performance optimization (to avoid object shape proliferation), and partly because it simplifies some of the inheritance code.