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

Add DJANGO_CT, DJANGO_ID, ID to be used with '__exact' internally. #232

Merged
merged 1 commit into from Mar 19, 2023

Conversation

karolyi
Copy link
Contributor

@karolyi karolyi commented Mar 17, 2023

Hey,

somewhere around 8568e49 an exclusion has been added to django_ct with an __exact search. I need to add the other django internal fields excluded from text searches as well.

In my use case I .exclude( some IDs for which I search for with the internal id field, that is a text field and did get escaped as a phrase because of that. My code uses SearchQuerySet.exclude(id__list=['id_1', 'id_2'] which in the current state will not work.

These commits are about that, and also about preparing a new release. Please do a release ASAP, as I need these changes and can't use the official packages until this gets merged. I'm using my on gitea mirror for now, but would like to get back to using the official packages.

@coveralls
Copy link

coveralls commented Mar 17, 2023

Coverage Status

Coverage: 97.565% (+0.007%) from 97.558% when pulling 0dcc958 on karolyi:master into 251e924 on notanumber:master.

@karolyi
Copy link
Contributor Author

karolyi commented Mar 18, 2023

@claudep hey, can we pick up some pace here please?

Copy link
Collaborator

@claudep claudep 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 patch. The release/version stuff will need to be in a separate PR dedicated to the next release. As for the rest of the changes, this will need a regression test.

@karolyi
Copy link
Contributor Author

karolyi commented Mar 18, 2023

I can remove the release, but I have no idea what you mean when you talk about the regression tests.

What I know though is, before the mentioned change in the commit, I could excloude ids, and now I can't.

@karolyi
Copy link
Contributor Author

karolyi commented Mar 18, 2023

I created a test for myself though (in my private system) that tests for this, maybe this can help:

from django.test.testcases import TestCase
from haystack.query import SearchQuerySet


class HaystackXapianBackendTestCase(TestCase):
    'Testing haystack with xapian backend.'

    def test_exclude_by_id(self):
        'Test if the ID field is not phrased.'
        # See https://github.com/notanumber/xapian-haystack/pull/232
        sqs = SearchQuerySet().exclude(id__in=['testing123', 'testing456'])
        expected = \
            'Query((<alldocuments> AND_NOT (Qtesting123 OR Qtesting456)))'
        self.assertEqual(first=str(sqs.query.build_query()), second=expected)

@claudep
Copy link
Collaborator

claudep commented Mar 18, 2023

Yes, this is a good start. Now you will have to include that test somewhere in https://github.com/notanumber/xapian-haystack/tree/master/tests/xapian_tests/tests.

@karolyi
Copy link
Contributor Author

karolyi commented Mar 18, 2023

Alright, I can do that, if you can promise a timely release.

@claudep
Copy link
Collaborator

claudep commented Mar 18, 2023

Depends on what you call "timely", do not forget we are volunteers!

@karolyi karolyi force-pushed the master branch 2 times, most recently from b6683e5 to 2edcdc0 Compare March 18, 2023 12:30
@karolyi
Copy link
Contributor Author

karolyi commented Mar 18, 2023

do not forget we are volunteers!

yeah, so am I. good news: I've added the test, reverted the version bump, and squashed the commits into one single, clean commit. tests are running, we should be good to go!

.gitignore Outdated Show resolved Hide resolved
Bump version

Fix syntax error

Adjust CHANGELOG.rst

Add test for __exact on ID
Copy link
Collaborator

@claudep claudep left a comment

Choose a reason for hiding this comment

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

Thanks!

@claudep claudep merged commit 90593c0 into notanumber:master Mar 19, 2023
26 checks passed
@karolyi
Copy link
Contributor Author

karolyi commented Mar 19, 2023

Nice, now all I need is a release.

Is that in your power to do?

@claudep
Copy link
Collaborator

claudep commented Mar 19, 2023

See PR #233

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.

None yet

3 participants