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

fix: auto capitalize target collection in by_ref_multi_target #1471

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

melsonic
Copy link

Copy link

@orca-security-eu orca-security-eu bot left a comment

Choose a reason for hiding this comment

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

Orca Security Scan Summary

Status Check Issues by priority
Passed Passed Infrastructure as Code high 0   medium 0   low 0   info 0 View in Orca
Passed Passed Secrets high 0   medium 0   low 0   info 0 View in Orca
Passed Passed Vulnerabilities high 0   medium 0   low 0   info 0 View in Orca

@weaviate-git-bot
Copy link

To avoid any confusion in the future about your contribution to Weaviate, we work with a Contributor License Agreement. If you agree, you can simply add a comment to this PR that you agree with the CLA so that we can merge.

beep boop - the Weaviate bot 👋🤖

PS:
Are you already a member of the Weaviate Slack channel?

@melsonic
Copy link
Author

Please review @tsmith023

@tsmith023
Copy link
Contributor

Hi @melsonic, this looks great to me from an implementation PoV. Would you be willing to add an integration test that validates this fix works as intended? We have many in the integration/ directory so feel free to add it there using the testing framework we have setup! Cheers 😁

@melsonic
Copy link
Author

Hi @melsonic, this looks great to me from an implementation PoV. Would you be willing to add an integration test that validates this fix works as intended? We have many in the integration/ directory so feel free to add it there using the testing framework we have setup! Cheers 😁

Yeah sure, I will add the required tests in the next commit.

@melsonic
Copy link
Author

Hi @tsmith023, Please review this PR when you have time.
Thank you!!

Copy link
Contributor

@tsmith023 tsmith023 left a comment

Choose a reason for hiding this comment

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

I like this test but it's a unit test rather than an integration test. Please could you move it to the test/collection/test_filter.py file? Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Collection name does not always auto capitalize the first letter
3 participants