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

feat: Adding StringJoiner #8357

Merged
merged 18 commits into from
Oct 30, 2024
Merged

feat: Adding StringJoiner #8357

merged 18 commits into from
Oct 30, 2024

Conversation

sjrl
Copy link
Contributor

@sjrl sjrl commented Sep 12, 2024

Related Issues

Proposed Changes:

How did you test it?

Notes for the reviewer

Checklist

@coveralls
Copy link
Collaborator

coveralls commented Sep 17, 2024

Pull Request Test Coverage Report for Build 11595719661

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.01%) to 90.117%

Totals Coverage Status
Change from base Build 11591750114: 0.01%
Covered Lines: 7723
Relevant Lines: 8570

💛 - Coveralls

@sjrl
Copy link
Contributor Author

sjrl commented Sep 26, 2024

Hey @silvanocerza this is one of the PR's that contains a pipeline behavioral test that fails currently but works in your subgraphs branch.

@sjrl
Copy link
Contributor Author

sjrl commented Oct 10, 2024

Once this PR #8431 is merged then the component in this PR will work and be ready for review.

@sjrl sjrl marked this pull request as ready for review October 30, 2024 13:48
@sjrl sjrl requested review from a team as code owners October 30, 2024 13:48
@sjrl sjrl requested review from dfokina and anakin87 and removed request for a team October 30, 2024 13:48
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.

Other than minor improvements, it looks good.

I would also like @silvanocerza to take a quick look.

haystack/components/joiners/string_joiner.py Outdated Show resolved Hide resolved
haystack/components/joiners/string_joiner.py Outdated Show resolved Hide resolved
@anakin87 anakin87 requested a review from silvanocerza October 30, 2024 14:15
Copy link
Contributor

@silvanocerza silvanocerza left a comment

Choose a reason for hiding this comment

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

Looks good to me too. 👍

@sjrl
Copy link
Contributor Author

sjrl commented Oct 30, 2024

Thanks @anakin87 ! I'll add a usage example.

@anakin87
Copy link
Member

@dfokina this component will probably go in 2.7.0 and we should create a simple doc page for it.

@sjrl sjrl requested a review from anakin87 October 30, 2024 14:28
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.

Feel free to merge after incorporating the suggestions for docstring
(I haven't checked if the format is OK)

haystack/components/joiners/string_joiner.py Outdated Show resolved Hide resolved
haystack/components/joiners/string_joiner.py Outdated Show resolved Hide resolved
Co-authored-by: Stefano Fiorucci <[email protected]>
@silvanocerza silvanocerza enabled auto-merge (squash) October 30, 2024 14:45
@silvanocerza silvanocerza merged commit 294a67e into main Oct 30, 2024
19 checks passed
@silvanocerza silvanocerza deleted the string-joiner branch October 30, 2024 15:03
silvanocerza added a commit that referenced this pull request Oct 30, 2024
* Adding StringJoiner

* Release notes

* Remove typing

* Remove unused import

* Try to fix header

* Fix one test

* Add to docs, move test to behavioral pipeline test

* Undo changes

* Fix test

* Update haystack/components/joiners/string_joiner.py

Co-authored-by: Stefano Fiorucci <[email protected]>

* Update haystack/components/joiners/string_joiner.py

Co-authored-by: Stefano Fiorucci <[email protected]>

* Provide usage example

* Apply suggestions from code review

Co-authored-by: Stefano Fiorucci <[email protected]>

---------

Co-authored-by: Stefano Fiorucci <[email protected]>
Co-authored-by: Silvano Cerza <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic:tests type:documentation Improvements on the docs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

feat: Add StringJoiner as a convenience component
4 participants