ruby / net-imap

Ruby client api for Internet Message Access Protocol
https://ruby.github.io/net-imap
Other
56 stars 29 forks source link

Add Config class for `debug`, `open_timeout`, and `idle_response_timeout` #291

Closed nevans closed 3 months ago

nevans commented 3 months ago

This enables:

This 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 compatibility load_defaults mentioned by #288 has not been implemented for this PR.

Two other significant implementation details, not originally mentioned in the #288 proposal:

nevans commented 3 months ago

@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 Config#load_defaults(version).

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 RawData, but that requires big updates to search (because currently complex searches need to be sent as a string). Those big updates will not be backported to 0.4, but I may backport the option to enable the deprecation warnings: it would still be useful to get warnings (and maybe convert that code to explicitly use RawData).

shugo commented 3 months ago

@nevans I'm not against adding Config, but why not just require it instead of autoload?

nevans commented 3 months ago

@nevans I'm not against adding Config, but why not just require it instead of autoload?

Good question. Config will always be loaded. 😆 I switched it to use require.

nevans commented 3 months ago

@shugo I did switch it to require_relative... but I forgot to push the version where I switched it (and a few other very minor documentation tweaks). I'll push that later today.

nevans commented 3 months ago

Pushed the autoload/require_relative switch in #295.