Adding a header parameters to the client constructor #771
Conversation
alallema
force-pushed
the
feature/add_header_option_on_client_constructor
branch
from
7353639 to
d79b5e4
Compare
meilisearch/_httprequests.py
Outdated
| "User-Agent": qualified_version() | ||
| + (";" + ";".join(config.client_agents) if config.client_agents else ""), |
There was a problem hiding this comment.
I'm not very satisfied with this line, if you know a better way, @sanders41 maybe, please lead the way!
There was a problem hiding this comment.
Does this work?
"User-Agent": ";".join(config.client_agents.append(qualified_version()))
Just ensure the client_agents is at least a [] array
There was a problem hiding this comment.
Right now the user agent is a string like: Meilisearch Python (v0.26.0);Meilisearch Package1 (v1.1.1);Meilisearch Package2 (v2.2.2)
Are you saying you want [Meilisearch Python (v0.26.0), Meilisearch Package1 (v1.1.1), Meilisearch Package2 (v2.2.2)]?
There was a problem hiding this comment.
I think he meant the client's input parameter, making adding several headers possible.
There was a problem hiding this comment.
The outcome should be a string, but until the last part (sending the data to the header) we could work with an array.
There was a problem hiding this comment.
Ok, if I'm understanding correctly this should be good then. The user specifies it as a tuple like ("Meilisearch Package1 (v1.1.1)", "Meilisearch Package2 (v2.2.2)") and can add as many as needed.
There was a problem hiding this comment.
Alright, these suggested changes are all based around the same idea of caching the user agent. Doing this means that the string only needs to be built once, and not for every request leading to better performance. All parameters to lru_cache need to be hashable so this is why I changed from List to Tuple (a bonus here is tuples are also more perfomant).
I couldn't really think of a cleaner way than what you did to build the string itself, so this suggestion is for performance, with the string building being mostly the same.
If you don't like this suggestion and don't want to use it it won't hurt my feelings 😄
Co-authored-by: Paul Sanders <[email protected]>
Co-authored-by: Paul Sanders <[email protected]>
Co-authored-by: Paul Sanders <[email protected]>
Co-authored-by: Paul Sanders <[email protected]>
Co-authored-by: Paul Sanders <[email protected]>
alallema
force-pushed
the
feature/add_header_option_on_client_constructor
branch
from
86a89b6 to
dce8fe0
Compare
No, I prefer this solution much more. Thanks a lot ❤️ |
meilisearch/_httprequests.py
Outdated
| @lru_cache(maxsize=1) | ||
| def _build_user_agent(self, client_agents: Optional[Tuple[str]] = None) -> str: | ||
| user_agent = qualified_version() | ||
| if not client_agents: | ||
| return user_agent | ||
|
|
||
| return f"{user_agent};{';'.join(client_agents)}" | ||
|
|
There was a problem hiding this comment.
You will want to move this out of the class or it can lead to memory leaks. If you want me to push an update so you can see what I mean let me know.
There was a problem hiding this comment.
Oh yes, I would like that, I'm not really sure where to put it.
meilisearch/_httprequests.py
Outdated
| @@ -19,7 +20,7 @@ def __init__(self, config: Config) -> None: | |||
| self.config = config | |||
| self.headers = { | |||
| "Authorization": f"Bearer {self.config.api_key}", | |||
| "User-Agent": qualified_version(), | |||
| "User-Agent": self._build_user_agent(config.client_agents), | |||
There was a problem hiding this comment.
| "User-Agent": self._build_user_agent(config.client_agents), | |
| "User-Agent": _build_user_agent(config.client_agents), |
There was a problem hiding this comment.
No because _build_user_agent shouldn't be part of the class
There was a problem hiding this comment.
The reason way is a bit hard to explain here, but this video does a really good job explaining it https://www.youtube.com/watch?v=sVjtp6tGo0g
|
Alright @alallema I pushed an update for you. |
Perfect! I think I get it now I didn't finish the video yet but it's already clearer ❤️ |
There was a problem hiding this comment.
Thanks @sanders41 for your inputs here and also for the video, very clear explanation!
LGTM! @alallema
|
bors merge |
|
Build succeeded:
|
meili-bors
bot
deleted the
feature/add_header_option_on_client_constructor
branch
To enable and facilitate the integration of a user-agent into all SDKs. see related issue and specifically for the docs-scraper integration a new parameter client_agent is now available in the client constructor.