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

Accept string based primary keys #163

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

Conversation

charles-vdulac
Copy link

[fixes issue #138 ]
[closes pr #157 ]

@coveralls
Copy link

coveralls commented Nov 19, 2016

Coverage Status

Coverage decreased (-0.06%) to 96.576% when pulling b7c8eed on charles-vdulac:master into f46283b on notanumber:master.

@coveralls
Copy link

coveralls commented Nov 19, 2016

Coverage Status

Coverage increased (+0.2%) to 96.799% when pulling 13c1e02 on charles-vdulac:master into f46283b on notanumber:master.

self.backend.update(UUIDBlogSearchIndex(), UUIDBlogEntry.objects.all())

self.assertEqual(pks(self.backend.search(xapian.Query(''))['results']),
[1, 2, 3])
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why isn't this ['uuid-1', 'uuid-2', 'uuid-3']? I thought that the goal was to ensure PK is a string.

@jorgecarleitao
Copy link
Collaborator

Overall, I like the proposed change a lot. Could you squash 13c1e02 into b7c8eed so we have a single commit with all the relevant changes regarding this new addition? It is also easier to review it.

@coveralls
Copy link

coveralls commented Nov 24, 2016

Coverage Status

Coverage increased (+0.1%) to 96.784% when pulling 7929436 on charles-vdulac:master into f46283b on notanumber:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage increased (+0.1%) to 96.784% when pulling 7929436 on charles-vdulac:master into f46283b on notanumber:master.

@coveralls
Copy link

coveralls commented Nov 24, 2016

Coverage Status

Coverage increased (+0.1%) to 96.784% when pulling 7929436 on charles-vdulac:master into f46283b on notanumber:master.

@jorgecarleitao
Copy link
Collaborator

jorgecarleitao commented Nov 24, 2016

I believe the PR is almost ready to merge.

As far as I understood, right now this tests the indexation (changes to test_backend), the ordering (test_order_by_django_id), and the correct generation of the query (test_filter_string_based_django_pk).

I believe that one final addition is to test the search interface, that is roughly present in test_interface.py. E.g. a test like this

self.assertEqual(pks(self.queryset.filter(id='uuid-1')),
                 pks(Document.objects.filter(django_id__eq='uuid-1')))

(see here for examples)

This would check that a search by id in Xapian corresponds to a search by id in Django's ORM, as we would expect.

You can also add your name to the credits in the README, as part of this commit.

Other than that, the code is clean and explicit, and, for me, this is ready to merge.

Thanks a lot @charles-vdulac for this!

Added test for search text-based ids.
@coveralls
Copy link

coveralls commented May 3, 2017

Coverage Status

Coverage increased (+0.5%) to 99.589% when pulling affe3d9 on charles-vdulac:master into f67036e on notanumber:master.

@coveralls
Copy link

coveralls commented May 3, 2017

Coverage Status

Coverage increased (+0.5%) to 99.589% when pulling affe3d9 on charles-vdulac:master into f67036e on notanumber:master.

@coveralls
Copy link

coveralls commented May 3, 2017

Coverage Status

Coverage increased (+0.5%) to 99.589% when pulling affe3d9 on charles-vdulac:master into f67036e on notanumber:master.

@charles-vdulac
Copy link
Author

Hi @jorgecarleitao Sorry for this long delay. I just merge your PR and rebased the branch to make it up to date.

@ciur
Copy link

ciur commented Oct 2, 2022

Would it be possible to make this pull request as part of the next release ?

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.

4 participants