-
Notifications
You must be signed in to change notification settings - Fork 27
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Pass request by name into methods #44
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we concerned about regression? Or do we trust future devs to inspect the history and understand that there is a reason (untested) for these positional args to be passed as keyword args?
It's a good point. We could sprinkle comments around explaining what's going on, but that feels like a bit of a rabbit hole. I think it's ok to just have the explanation in the git/github history. |
Would a test be overkill? A regression would fail the test, which would make clear that those args are keyword-ed for a reason. |
No a test makes sense, I'll add something that mimics what the PC is doing. |
This makes #22 less breaking.
23d6db8
to
1708e8e
Compare
Whelp, that test ended up being more involved than expected, but got it. |
Related Issue(s):
Description:
Some downstreams, e.g. the Planetary Computer, use the
request
as a part of their caching mechanism. #22 has some internal calls to functions whererequest
is passed as a positional parameter, which breaks generic caching mechanism, which fetchrequest
fromkwargs
. This PR changes to make sure those internal calls passrequest
by name.PR Checklist:
pre-commit
hooks pass locallymake test
)make docs
)