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

Ensure config.path = None and config.path missing mean the same thing #704

Merged
merged 1 commit into from
May 25, 2024

Conversation

ianawilson
Copy link
Contributor

@ianawilson ianawilson commented May 1, 2024

Previously if path was present, it was expected to have a value. This allows path to be None, and for None and empty to act in the same way.

Originally I made this change because setting defaults to False and keyword to True didn't seem to be enough for me.

@davidmezzetti
Copy link
Member

Just for my clarity, the PR supports adding a a config key of path: None? The current method assumes path wouldn't be there but this change takes it a step further and checks that if the key is there, it has a value?

@ianawilson ianawilson changed the title Allow config.path = None and config.keyword = True for full text search only Ensure config.path = None and config.path missing mean the same thing May 14, 2024
@ianawilson
Copy link
Contributor Author

Just for my clarity, the PR supports adding a a config key of path: None? The current method assumes path wouldn't be there but this change takes it a step further and checks that if the key is there, it has a value?

We talked a bit in community slack, but the gist is that I wanted None and missing to act the same. The title and description have been updated for clarity.

@ianawilson ianawilson marked this pull request as ready for review May 14, 2024 17:52
@davidmezzetti davidmezzetti added this to the v7.2.0 milestone May 25, 2024
@davidmezzetti davidmezzetti merged commit e1ee9d8 into neuml:master May 25, 2024
0 of 3 checks passed
@davidmezzetti
Copy link
Member

Thank you, appreciate the improvement!

davidmezzetti added a commit that referenced this pull request May 25, 2024
davidmezzetti added a commit that referenced this pull request May 25, 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.

2 participants