[gigeresk]

#339 added a version lock to hishel<2.0 because hishel 2.0 was released today and this breaks githubkit installation. This PR attempts to update the source to reflect hishel's 2.0 API changes.

[yanyongyu]

I'd consider this a new feature adding support for hishel v2, rather than a githubkit installation fix.

[gigeresk]

@yanyongyu sure, the fix is here #339. All that to say, currently installation from pypi is broken.

uv run --isolated --no-project --with githubkit python -c "import githubkit; print('ok')"
# workaround:
uv run --isolated --no-project --with githubkit --with 'hishel<2' python -c "import githubkit; print('ok')"

[mgkvibez]

Reviewing this as a migration checklist, since I checked the new signature directly:

CacheTransport(next_transport, storage: BaseStorage | None = None, rfc: bool | CacheOptions = False)
AsyncCacheTransport(next_transport, storage: AsyncBaseStorage | None = None, rfc: bool | CacheOptions = False)

The CachePolicy → rfc/CacheOptions mapping and the SyncSqliteStorage → SqliteStorage rename both look right, and keeping AsyncBaseStorage in the annotation is correct, it still exists in 2.0.

One thing I would change before this lands:

The default flipped from None to True, which is not parity. Previously kwargs.pop("policy", None) meant "no policy"; now kwargs.pop("policy", True) goes into rfc, and rfc=True opts the transport into RFC-9111 heuristic freshness, so responses carrying validators can be served from cache where they previously would not have been. hishel's own default for rfc is False. If the intent is to preserve existing behaviour, False is the value matching the old None and hishel's default; if True is deliberate, it deserves a changelog line, because it silently changes caching for everyone who never passed policy.

Two smaller notes:

1. The public kwarg keeps the name policy but changes type from CachePolicy | None to CacheOptions | bool. A caller passing a CachePolicy object now gets a TypeError at transport construction rather than something readable, so a line in the migration notes would save someone ten minutes.
2. GitHub reports the PR as non-mergeable against master right now, most likely uv.lock. A rebase would let CI reflect the actual merge result.

If it is useful, I can test the caching path against a real conditional request, since the default question is the only part I cannot settle from the diff alone.

[gigeresk]

@mgkvibez I addressed your smaller notes but I disagree that kwargs.pop("policy", None) meant "no policy", since there's a default . e.g.

uv run --isolated --no-project --with 'hishel[httpx]==1.4.0' python -c "
import httpx
from hishel.httpx import SyncCacheTransport
t = SyncCacheTransport(next_transport=httpx.HTTPTransport(), policy=None)
print(type(t._cache_proxy.policy).__name__)
"

prints

SpecificationPolicy

[mgkvibez]

You're right and I was wrong. I checked it against hishel 1.4.0 rather than trusting my reading:

from hishel.httpx import SyncCacheTransport
SyncCacheTransport(next_transport=httpx.HTTPTransport(), policy=None)._cache_proxy.policy
# SpecificationPolicy

which is the same object the no-argument constructor gives. So kwargs.pop("policy", None) never meant "no policy", and my parity claim against it was simply incorrect. Sorry for the noise on that one.

Looking closer, the mapping you chose is the right one, and for a reason worth recording in the migration note: 2.0's own default is not the spec path. In hishel/httpx/_sync.py, rfc=False takes the legacy branch (identity is f"{request.method} {request.url}", no RFC semantics), and only rfc=True or a CacheOptions goes through _handle_with_rfc. So policy=True → rfc=True is what reproduces 1.x's SpecificationPolicy, while leaving it at the library default would have silently moved githubkit onto the legacy cache key. Your _check_policy guard plus the note covers the type change well.

One residual difference, which I am flagging as an observation rather than a defect since I have already been wrong once in this thread: 2.0's _core/_spec.py implements RFC 9111 §4.2.2 heuristic freshness as priority 4 (derived from Last-Modified), and 1.4.0's SpecificationPolicy source contains no heuristic calculation at all. So users who relied on the old default may see some responses treated as fresh where they previously were not. Whether that deserves a sentence in the migration note is your call, not a blocker from my side.

[gigeresk]

@mgkvibez I suspect that you're a bot or are just using an LLM to review the code so I will disregard your comments from now on.
@yanyongyu

[gigeresk]

@mgkvibez I suspect that you're a bot or are just using an LLM to review the code so I will disregard your comments from now on.
@yanyongyu

[mgkvibez]

None of that tbh.. I just don't do much of composing and typing a lot so I have an llm at hand who always restructure my reviews but if you want to take it like that it's fine you can do what you want @gigeresk

[yanyongyu]

good work! some small suggestions

[yanyongyu]

This method is still called policy. We may add a new method get_hishel_options with get_hishel_policy backport or just remove it.

[yanyongyu]

Seems need reformat, remove the comma

[yanyongyu]

the arg name policy is outdated. this should be rfc, align with hishel v2.

[yanyongyu]

this may be unnecessary.

[yanyongyu]

remove comma here