fix: don't require api_url/api_key in local mode#8
Merged
Conversation
The api_url check ran unconditionally and before the local_mode logic, so ClientOptions(local_mode=True) raised "api_url must be provided". That defeats the purpose of local mode: a client that never reaches the network had to be handed an endpoint it would never use. Gate both endpoint checks on local_mode being off. Also drop the duplicate guard in BaseClient.__init__, which raised RuntimeError for the missing-key case that __post_init__ already rejects with ValueError.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to #7, as flagged in the review there.
The
api_urlcheck added in cdac5d0 ran unconditionally and sat before the local_mode logic, so:That defeats the point of local mode. A client that never touches the network had to be handed an endpoint it would never use, since
log()returns early anyway. The existing tests didn't catch it because they all pass anapi_url.Both endpoint checks are now gated on local_mode being off. Also removed the duplicate guard in
BaseClient.__init__, which raisedRuntimeErrorfor the missing-key case that__post_init__already rejects withValueError(unreachable, and a second exception type for the same condition).Behavior now:
ClientOptions(local_mode=True)ClientOptions(local_mode="if_unset_api_key")ClientOptions(api_key="lp_k")ValueError: api_url must be providedClientOptions(api_url="http://x:8080")ValueError: api_key must be provided...Added a parametrized test covering local mode with nothing configured.
Suite: 138 passed, 8 skipped. The 2 failures in
tests/middleware/test_imports.pyare pre-existing on main and unrelated.