Merge headers if passed via client_kwargs in query client
#958
Conversation
Kludex
changed the title
client_kwargs
Deploying logfire-docs with
|
| Latest commit: |
d16dba7
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://18a17c22.logfire-docs.pages.dev |
| Branch Preview URL: | https://please-merge-2.logfire-docs.pages.dev |
There was a problem hiding this comment.
Pull Request Overview
This PR ensures that headers passed via client_kwargs are merged with the default 'authorization' header in the query client. Key changes include:
- Extracting headers from client_kwargs.
- Merging the provided headers with the default authorization header.
- Passing the updated headers to the client instance.
| timeout=timeout, base_url=base_url, headers={'authorization': read_token}, **client_kwargs | ||
| ) | ||
| headers = client_kwargs.pop('headers', {}) | ||
| headers['authorization'] = read_token |
There was a problem hiding this comment.
This line unconditionally sets the 'authorization' header, potentially overwriting an existing header provided via client_kwargs. Consider using headers.setdefault('authorization', read_token) or explicitly checking for an existing value to preserve any intended header overrides.
| headers['authorization'] = read_token | |
| headers.setdefault('authorization', read_token) |
Copilot is powered by AI, so mistakes are possible. Review output carefully before use.
There was a problem hiding this comment.
this isn't a terrible point. read_token should probably take priority but if there's a clash maybe we should emit a warning or something. but it doesn't actually matter.
| self.client: T = client( | ||
| timeout=timeout, base_url=base_url, headers={'authorization': read_token}, **client_kwargs | ||
| ) | ||
| headers = client_kwargs.pop('headers', {}) |
There was a problem hiding this comment.
i'd slightly prefer if headers was just a normal arg.
alexmojaki
changed the title
client_kwargsclient_kwargs in query client
No description provided.