Require octokit/rate_limit from lib/octokit/error.rb so RateLimit is available

[timrogers]

Octokit::Error#build_error_context uses RateLimit.from_response but does not require the file for RateLimit explicitly.

This can lead to cases where the RateLimit module is not available when needed, causing an error.

This requires the file explicitly where the module is used.

Fixes #1422.

[timrogers]

The underlying issue here was introduced by #1351, which switched from loading all of Octokit when require 'octokit' is called to lazy-loading parts of the gem the first time Octokit::Client is referenced.

The end result is that if you instantiate an Octokit::Error subclass before you refer to Octokit::Client, it will fail because Octokit::RateLimit hasn't been loaded. (That's because it's requiring octokit/client which ultimately leads to RateLimit being required.)

It's likely that there will be other similar cases, but it's hard to trace all of them because of the complex tree of how things are required. It seems reasonable just to fix this one case for now, rather than throw the baby out with the bathwater and revert the autoloading.

[timrogers]

Looking at other recent PRs, #1420 is actually address the same problem, but affecting Octokit::Repository instead of error classes.

This makes me wonder if lazy-loading is just a bit problematic for users.

Lazy-loading means that anyone referring to a module inside Octokit directly has to manually require the relevant file in order to be sure that it's available. This is all the more dangerous as you have to refer to a file path, effectively making internal file paths part of the SDK's public API.

At the same time, it would be a shame to lose lazy-loading, as it trims a fair bit of time from the boot-up of people's apps per #1351.

I wonder if we should offer users the choice of eager loading and lazy loading. One way to do that would be to add a lib/octokit/all.rb file which requires everything, and then people could do the following in their Gemfile:

gem 'octokit', '4.23.0', require: 'octokit/all'

If we do this, the most interesting question is "what should be the recommended approach in the readme?". Should we suggest that people require everything and offer lazy-loading as a power user feature? Or should we recommend the lazy-loading approach and offer "requiring everything" is an advanced option for people delving into internals a bit more?

I prefer the former to make the experience of using Octokit as smooth as possible, especially given that referring to classes inside the gem is likely to be a common case - especially for errors which you rescue.

[timrogers]

After further discussion with @nickfloyd, I think the right option is to revert #1420 for now as it is presenting itself as a breaking change in a few places. We can then work out a better, opt-in way to do lazy-loading.

[kennyadsl]

I think the right option is to revert #1420

@timrogers do you mean #1351?

[timrogers]

I think the right option is to revert #1420

@timrogers do you mean #1351?

I did! Apologies for my mistake 😓

[timrogers]

Closing in favour of #1428.