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

Support exclusion file filters #826

Open
wants to merge 12 commits into
base: master
Choose a base branch
from

Conversation

srikary12
Copy link

@debanjum debanjum changed the title Feature: Added file filters Support exclusion file filters Jun 19, 2024
Copy link
Member

@debanjum debanjum left a comment

Choose a reason for hiding this comment

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

Hey @srikary12, just left 1 comment (to add tests) but otherwise changes look good! Excited to get file filters to word filter parity

src/khoj/search_filter/file_filter.py Show resolved Hide resolved
@srikary12
Copy link
Author

@debanjum Can you check and let me know if it's working fine.

@srikary12 srikary12 marked this pull request as draft June 24, 2024 13:42
file filter test cases
@srikary12 srikary12 marked this pull request as ready for review June 24, 2024 13:44
@srikary12
Copy link
Author

@debanjum @sabaimran let me know if there are any issues

@srikary12
Copy link
Author

@debanjum added the case

Copy link
Member

@debanjum debanjum left a comment

Choose a reason for hiding this comment

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

Thanks for the working through the feedback! I've left some comments to fix the unit tests. Maybe a good idea to manually validate these changes locally and/or run the unit tests on your machine with pytest. It'll speed up the review cycle

def test_exclude_file_filter_terms():
# Arrange
file_filter = FileFilter()
q_with_filter_terms = 'head tail file:-"file 1.org" file:"/path/to/dir/*.org"'
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't the tested query be something like below?

Suggested change
q_with_filter_terms = 'head tail file:-"file 1.org" file:"/path/to/dir/*.org"'
q_with_filter_terms = 'head tail -file:"file 1.org" file:"/path/to/dir/*.org"'

def test_file_exclude_filter():
# Arrange
file_filter = FileFilter()
q_with_exclude_filter = 'head -"exclude_file" tail'
Copy link
Member

Choose a reason for hiding this comment

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

The query tested seems to be for word filters instead of file filters. Wouldn't the query with file filter be of the form:

Suggested change
q_with_exclude_filter = 'head -"exclude_file" tail'
q_with_exclude_filter = 'head -file:"/path/to/excluded_file" tail'

def test_file_include_and_exclude_filter():
# Arrange
file_filter = FileFilter()
q_with_include_and_exclude_filter = 'head "include_file" -"exclude_file" tail'
Copy link
Member

Choose a reason for hiding this comment

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

The query tested seems to be for word filters instead of file filters. Wouldn't the query with file filter be of the form:

Suggested change
q_with_include_and_exclude_filter = 'head "include_file" -"exclude_file" tail'
q_with_include_and_exclude_filter = 'head file:"include_file" -file:"exclude_file" tail'

@@ -87,6 +87,42 @@ def test_get_file_filter_terms():
assert filter_terms == ["file 1\\.org", "/path/to/dir/.*\\.org"]


def test_exclude_file_filter_terms():
Copy link
Member

Choose a reason for hiding this comment

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

A more appropriate name for this unit test maybe test_include_and_exclude_file_filter_terms

Suggested change
def test_exclude_file_filter_terms():
def test_include_and_exclude_file_filter_terms():

@srikary12
Copy link
Author

Sure, I'll be working on it.

Thanks.

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.

[IDEA] Support exclusion file filters
2 participants