-
Notifications
You must be signed in to change notification settings - Fork 128
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
docs: explain different connection string formats in the docstring #1132
docs: explain different connection string formats in the docstring #1132
Conversation
integrations/pgvector/src/haystack_integrations/document_stores/pgvector/document_store.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks @kanenorman for your contribution! I just left a few minor comments
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kanenorman I am sorry to step in after you have worked on this PR.
I think that the original issue proposes an acceptable workaround for the problem. (I tried it and it also works on default use cases)
After some discussion, we think that it is better to explain the possible connection string formats in the docstring instead of introducing a new init parameter, which might complicate the API and make future maintenance more difficult.
Therefore, my proposal is to:
- remove the
connection_param_kwargs
init parameter - make the
connection_string
docstring more comprehensive, including an example like this:"host=HOST port=PORT dbname=DBNAME user=USER password=PASSWORD"
.
No problem @anakin87! Thank you for stepping in. I reverted the changes and enhanced the docstrings in the latest commit. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
Related Issues
Proposed Changes:
Allow user to provide connection parameters as a dictionary of individual arguments
How did you test it?
to_dict()
andfrom_dict()
Notes for the reviewer
This PR is labeled as a feature rather than a bug because the underlying library, rather than Haystack itself, contains the bug.
Checklist
fix:
,feat:
,build:
,chore:
,ci:
,docs:
,style:
,refactor:
,perf:
,test:
.