Skip to content

Add custom text-property and set default to previously used "text" #2959

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

Conversation

Temuu-jin
Copy link
Contributor

@Temuu-jin Temuu-jin commented May 1, 2025

The link to sign contributor license is not working,
Im not sure what is meant by squash your commits,
no tests or anythng updated, just simply added the property to the builder and class as well as replaced hard coded "text" with property where needed,
not sure how to run a build. normally i work on my macbook but currently on windows and nothing works...

any help/guidance as to how i should proceed would be appreciated since ive never contributed on github

Signed-off-by: Temucin Damdinjamts-Kintaert [email protected]

@Temuu-jin Temuu-jin marked this pull request as ready for review May 1, 2025 10:03
@Temuu-jin
Copy link
Contributor Author

#2687

@Temuu-jin
Copy link
Contributor Author

@meistermeier I'm sorry, I am so lost on GitHub GUI, not sure how to link the issue and make you reviewer

@meistermeier
Copy link
Contributor

Looks good so far. Could you also add support for the text property in Neo4jVectorStoreAutoConfiguration?
Something like this (and the needed field in the Neo4jVectorStoreProperties):

.textProperty(properties.getTextProperty())

@Temuu-jin
Copy link
Contributor Author

will update PR by wednesday latest, is that okay timewise? if you need it faster i can reprioritize

@markpollack markpollack added this to the 1.0.0-RC1 milestone May 6, 2025
@Temuu-jin
Copy link
Contributor Author

Looks good so far. Could you also add support for the text property in Neo4jVectorStoreAutoConfiguration? Something like this (and the needed field in the Neo4jVectorStoreProperties):

.textProperty(properties.getTextProperty())

PR commit added :D

Copy link
Contributor

@meistermeier meistermeier left a comment

Choose a reason for hiding this comment

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

(!) I think you still need to sign off the commits within your commit to pass the contributors guidelines.
Besides the formatting for this line, all looks good from my side.

Comment on lines 77 to 79
.constraintName(properties.getConstraintName())
.textProperty(properties.getTextProperty())
.build();
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
.constraintName(properties.getConstraintName())
.textProperty(properties.getTextProperty())
.build();
.constraintName(properties.getConstraintName())
.textProperty(properties.getTextProperty())
.build();

Signed-off-by: Temucin Damdinjamts-Kintaert <[email protected]>
@Temuu-jin
Copy link
Contributor Author

updated the pr, tried to sign off via rebase but I keep getting permission denied using my generated github pat key

Copy link
Contributor

@meistermeier meistermeier left a comment

Choose a reason for hiding this comment

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

Approved from the technical side. Since you already have signed-off one commit, it might be ok for @markpollack to squash and merge it.

@ilayaperumalg
Copy link
Member

@Temuu-jin Thanks for the PR! @meistermeier Thanks for the review!

@ilayaperumalg
Copy link
Member

Fixed the formatting issue, rebased and merged as 972eb49

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

Successfully merging this pull request may close these issues.

4 participants