fix: WithContext had no effect

[armsnyder]

Fixes #1336

[FedorLap2006]

Hi, thanks for outlining the problem. Haven't noticed it myself when implementing the options.
However I'm not sure this solution would be correct one, as it's not entirely consistent with how other options are implemented.

Looking through the code of RequestWithLockedBucket, it uses cfg.Client.Do(req) for making a request. If that line would be replaced by cfg.Client.Do(cfg.Request) this problem would be solved.

[armsnyder]

@FedorLap2006 I still prefer my first version, but I changed it to reassign req in the caller function instead. I didn't want to simply replace cfg.Client.Do(req) with cfg.Client.Do(cfg.Request) since there are more places that req is used.

[FedorLap2006]

I didn't want to simply replace cfg.Client.Do(req) with cfg.Client.Do(cfg.Request) since there are more places that req is used.

My mistake, yes, there were a few other places that it was used in.
In that case, all occurences should be replaced by cfg.Request then. Reassignment is fine, but might be confusing when reading the function from the point and forward, the cfg.Request on other hand clearly communicates we're using the request modified by the With functions.

[armsnyder]

@FedorLap2006 Hi, I followed your suggestion and updated the code. ☺️

My two cents are the first version with *cfg.Request = *cfg.Request.WithContext(ctx) is the cleaner and more consistent option.

More consistent: All the other RequestOption's are mutating the original request. It seems that WithContext should also mutate it.

Cleaner: It's better to have only one request in-scope in the RequestWithLockedBucket function, instead of keeping a reference to the old req before applying options. It is a long function (111 lines!), so it may not be obvious for future code changes whether req or cfg.Request should be used.

You have final say, of course. 😅

[FedorLap2006]

Sorry for taking matter into my own hands. After thinking for a while, the second solution (with reassigning req), does not affect readability that much. And has the least amount of changes. Therefore I've reverted the replacement of cfg.Request

P.S. Another way this could have been implemented was to use a Context field in RequestConfig and then reassign the req to req.WithContext(cfg.Request) (which is what I originally wanted to do), but req = cfg.Request would just be a simpler version of this.

[FedorLap2006]

Sorry for commiting right on the branch. Since the release is planned for today, I've decided to not prolong the things even more and apply all the edits myself.

Thank you for the contribution!

[armsnyder]

@FedorLap2006 No worries! Ultimately what matters most is the bug is fixed. 😄 Thanks for the review.