Issue #1566 - add support for a @hookable decorator to HTTPClient.request() (extensible elsewhere)

[bklaas]

Humbly submitting this PR for review from the gspread team.

Problem:
We use gspread extensively to speak to the Google Sheets API. As an org use it heavily enough to frequently hit miscellaneous API limits set by Google for this service. We'd like to be able to have more visibility when rate limit and other issues arise.

Proposed solution:
The PR contains a new Hookable class, which feeds into a @hookable decorator for use wherever callback hooks might be used. In this case, I've added @hookable to the HTTPClient.request() method, which in turn is called by BackoffHTTPClient, the client we use to get work done in gspread.

With @hookable added to request(), this empowers downstream code to add hooks that can be called under the following circumstances:

• before the method is called
• if a retry error response was received
• if a timeout has been triggered
• if an exception was hit in the method
• when the method finishes

Testing:

Unit tests have been added and run against all new hooks.

Notes:

I discovered the set_timeout pytest test sets the timeout, but this bleeds over to the remaining tests. For this reason, the test was modified to set the timeout back to the original value at the conclusion of that test (which by default is None)

In my own testing, I had to skip the two tests that called .openall which, for my org's API key, results in a method that never returns. We have thousands of Sheets documents so that method scales really poorly in this use case.

[bklaas]

Also meant to note in the description -- this is a no-op for a downstream user not using any hooks. No change in behavior if you are not using the feature.

[alifeee]

hi :]

thanks for the PR. it looks like useful stuff !

the code and tests look reasonable to me, and look like they could help you with your use-case

as part of PRs to gspread, I have some questions to ask before continuing:

• were AI or AI code generation tools used in the creation of this pull request?
• is there a future maintenance burden
• could there be any examples added to the documentation

thanks again for your work :]

p.s. you can check that the checks will pass or fail locally with

pip install tox
tox -e lint
tox -e py

[bklaas]

linting was failing for a single miss-sort of an import: fixed.
testing was failing, only on github, I believe because I hadn't committed the VCR cassettes created for the new tests. They have been added.

Answering your questions:

• were AI or AI code generation tools used in the creation of this pull request?

I will answer this with a qualified yes. I do not "vibe code", in other words I don't allow AI code generation to be a "first-class citizen" when writing code. I drive it, not the other way around. But transparently, I do use AI tools to do a few things: working through specific syntax issues, giving me first-drafts of docstrings (though I read and edit all of them), and boilerplates for initial unit tests (again, I heavily review/rewrite).

• is there a future maintenance burden

My intention with building this as a decorator was to reduce future maintenance burden. It contains Hookable functions in its own area, which would e.g. make removing it very simple-- remove the @hookable decorator from HTTPClient.request and the entire contribution would be effectively turned off.
I won't say it's zero maintenance burden, as all production code has a non-zero maintenance burden, but it should be very small.

• could there be any examples added to the documentation

Good idea. I checked in a block of description of how to use callback hookd in docs/advanced.rst.

[bklaas]

On my fork I have one test that is failing. All tests pass locally.

I believe this PR is fully ready, but I don't know how to resolve that workflow failure. I tried checking in VCR cassette files for the hookable tests, but the workflow fails on one test. If you have any insight it would be much appreciated.

[bklaas]

CI issue solved. I had a problematic and somewhat superfluous test that I removed. All CI passes now.

[alifeee]

Hi @bklaas

Thanks again for the feature request. The example you wrote for the docs is, to me, a good example of what feature you think would be useful.

As for the code, it seems to me to look generic (it's just called "hookable" for example), but the code inside is quite specific (it talks of retry/timeout, uses HTTP codes). My point being that it would seem that you wanted to make something generic ("hookable") but it seems to be specific to the flow and errors of an HTTP request. If this is intentional, then this is intentional. I don't spend so much time in the source code of gspread, so it confuses me a little to see this generic+specific combo.

However, as you say, the feature is easily toggleable, and seems useful to me.

I'd like to wait a small time for a comment from @lavigne958 but then I would be happy to merge this.

Thanks for your (& the AI's) work :]

[bklaas]

Hi @alifeee , thanks for the time in reviewing this PR again. Your critique is valid on generic vs specific. I did write it initially to be a generic decorator, but I would agree the finished product is specific to HTTP requests. I think the best thing to do would be to change the name of the decorator and class.

I am going to rename these, with no other changes to the code:
Hookable -> RequestHookMixin
@hookable -> @with_hooks

[bklaas]

Bumping this after a couple of weeks. Any thoughts on merging this @alifee / @lavigne958 ?

[alifeee]

Bumping this after a couple of weeks. Any thoughts on merging this @alifee / @lavigne958 ?

I have had no more thoughts. I'm happy to merge it. I do not know when it will be released as part of a release. I have not seen anyone but yourselves ask for it, so I am assuming that you can patch your own library in the meanwhile. gspread development is a little slow at the moment, as there are no active maintainers (I am only really triaging)