Skip to content
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

Merged
merged 9 commits into from
Oct 15, 2024

Conversation

kanenorman
Copy link
Contributor

Related Issues

Proposed Changes:

Allow user to provide connection parameters as a dictionary of individual arguments

How did you test it?

  • Ran existing test
  • Updated existing test
  • Added unit test for to_dict() and from_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

@kanenorman kanenorman requested a review from a team as a code owner October 10, 2024 23:58
@kanenorman kanenorman requested review from davidsbatista and removed request for a team October 10, 2024 23:58
@github-actions github-actions bot added integration:pgvector type:documentation Improvements or additions to documentation labels Oct 10, 2024
Copy link
Contributor

@davidsbatista davidsbatista left a 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

@anakin87 anakin87 self-requested a review October 11, 2024 09:03
Copy link
Member

@anakin87 anakin87 left a 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".

@kanenorman
Copy link
Contributor Author

kanenorman commented Oct 14, 2024

No problem @anakin87! Thank you for stepping in. I reverted the changes and enhanced the docstrings in the latest commit.

Copy link
Member

@anakin87 anakin87 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@anakin87 anakin87 changed the title feat: specify individual connection parameters docs: explain different connection string formats in the docstring Oct 15, 2024
@anakin87 anakin87 merged commit a223e6f into deepset-ai:main Oct 15, 2024
7 checks passed
@kanenorman kanenorman deleted the postgres-connection-params branch October 15, 2024 13:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
integration:pgvector type:documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bug/Workaround/feature proposal : DBString Parsing in PgvectorDocumentStore
3 participants