Skip to content

Move GemFire configuration to inner class#1168

Closed
jhuynh1 wants to merge 1 commit into
spring-projects:mainfrom
jhuynh1:duperemove
Closed

Move GemFire configuration to inner class#1168
jhuynh1 wants to merge 1 commit into
spring-projects:mainfrom
jhuynh1:duperemove

Conversation

@jhuynh1

@jhuynh1 jhuynh1 commented Aug 5, 2024

Copy link
Copy Markdown
Contributor

Added builder and moved config class as an inner class of GemFireVectorStore to match other vector store projects

Thank you for taking time to contribute this pull request!
You might have already read the [contributor guide][1], but as a reminder, please make sure to:

  • Sign the contributor license agreement
  • Rebase your changes on the latest main branch and squash your commits
  • Add/Update unit tests as needed
  • Run a build and make sure all tests pass prior to submission

@markpollack markpollack added this to the 1.0.0-M2 milestone Aug 16, 2024
return new Builder();
}

public static class Builder {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

In many of our other vector store implementations, the builder is provided as an inner class of the configuration. Why do you have it outside of GemFireVectorStoreConfig?

@sobychacko

Copy link
Copy Markdown
Contributor

@jhuynh1 As a general comment, please add your name as an author to all the classes you modified.

@markpollack markpollack modified the milestones: 1.0.0-M2, 1.0.0-RC1 Aug 20, 2024
sobychacko added a commit to sobychacko/spring-ai that referenced this pull request Aug 22, 2024
- Based on the pattern established in other vector store support implementations,
  added a builder class as an inner class of the GemfireVectorStoreConfig class which
  is also moved as an inner class to GemfireVectorStore.

Based on the original PR: spring-projects#1168
@sobychacko

Copy link
Copy Markdown
Contributor

@jhuynh1 Due to the latest changes in main, this PR branch has diverged from the main, and I had merge conflicts on rebase. Started from scratch based on your changes and addressed the review comments. The changes are here. Closing the PR.

@sobychacko sobychacko closed this Aug 22, 2024
@sobychacko sobychacko modified the milestones: 1.0.0-RC1, 1.0.0-M2 Aug 22, 2024
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.

3 participants