Skip to content

ENH: allow for custom controller.#19

Closed
cournape wants to merge 2 commits intopsf:masterfrom
cournape:custom_controller
Closed

ENH: allow for custom controller.#19
cournape wants to merge 2 commits intopsf:masterfrom
cournape:custom_controller

Conversation

@cournape
Copy link
Copy Markdown
Contributor

This simple PR allows for an adapter to take a custom controller.

The rationale is that for legacy reasons, I may have to support etag caching where I ignore the query string (think secured s3 entries). The simplest way seems to have my own controller subclass that passes a different key to the underlying cache

Comment thread cachecontrol/adapter.py Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's call this controller_class instead.

@ionrock
Copy link
Copy Markdown
Contributor

ionrock commented Apr 10, 2014

I like this idea! Thanks for submitting the pull request. If we can make the small stylistic changes above, then I'm happy to merge this and push it out.

Great work!

@cournape
Copy link
Copy Markdown
Contributor Author

@ionrock thanks for the quick review. Comments addressed.

@cournape
Copy link
Copy Markdown
Contributor Author

I added an example of this there https://gist.github.com/cournape/10432466

@ionrock
Copy link
Copy Markdown
Contributor

ionrock commented Apr 11, 2014

I merged this manually so I could squash your two commits into one (07ef57a). Unfortunately, that removed a bit of metadata giving you credit.

Would you mind please sending me a pull request adding your name and/or github username to the CONTRIBUTORS.rst? I hate to look like I took credit for your work ;)

Thanks again!

@ionrock ionrock closed this Apr 11, 2014
@cournape
Copy link
Copy Markdown
Contributor Author

@ionrock don't worry about it. I may prepare a PR with an actual example, though.

@ionrock
Copy link
Copy Markdown
Contributor

ionrock commented Apr 11, 2014

That would be great! If you want to include it in the docs, that would
be even better. Thanks!

David Cournapeau notifications@github.com writes:

@ionrock don't worry about it. I may prepare a PR with an actual example, though.


Reply to this email directly or view it on GitHub:
#19 (comment)

Sent with my mu4e

@hexagonrecursion
Copy link
Copy Markdown
Contributor

IMHO merging this was a mistake. While this is a small change it has big implications: it makes the interface of controller.CacheController part of the public API and any changes to it are potentially breaking. This use case should have been addressed with something that does not create such a big commitment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants