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

Make load_all() work with primary keys that are objects #1115

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

Conversation

linkmic
Copy link

@linkmic linkmic commented Nov 27, 2014

Required to support django-uuidfield as primary key.

…just assuming that they are string or int. Required to support django-uuidfield as primary key.
@acdha acdha changed the title Make load_all() work with primary keys that are objects, rather than jus... Make load_all() work with primary keys that are objects Dec 8, 2014
@acdha
Copy link
Contributor

acdha commented Dec 8, 2014

That looks great – any chance you could contribute a test which would break currently?

@linkmic
Copy link
Author

linkmic commented Dec 8, 2014

Hi,

I can give a test a crack, but it would make your tests depend on
django-uuidfield. Is that OK?

Also if you have a preference as to which test module you'd like the test
in, etc. then let me know.

Thanks,
/mike
On Dec 8, 2014 6:56 PM, "Chris Adams" [email protected] wrote:

That looks great – any chance you could contribute a test which would
break currently?


Reply to this email directly or view it on GitHub
https://github.com/toastdriven/django-haystack/pull/1115#issuecomment-66156436
.

@linkmic
Copy link
Author

linkmic commented Dec 16, 2014

I have added a test now. It introduces a dependency on django-uuidfield unfortunately. Tell me if you can see a better way.

@linkmic
Copy link
Author

linkmic commented Dec 17, 2014

The python 3.3 tests are failing currently because django-uuidfield uses the unicode() method without checking Python version. I have made an issue in that project and will monitor for a fix.
dcramer/django-uuidfield#51

@linkmic
Copy link
Author

linkmic commented Dec 17, 2014

I can successfully run the tests locally using Python 3.4 and Django 1.6.0, provided I use the unreleased master branch of django-uuid which contains fixes for Python3.
dcramer/django-uuidfield@ec4f97f

I'd propose waiting for a new release of django-uuidfield before merging this pull request.

@acdha
Copy link
Contributor

acdha commented Dec 17, 2014

That seems reasonable – tempting as it is to have a simple test class for this, it should be less maintenance long-term to have a simple PyPI install

…bjects. Also, it modifying a dictionary that is being iterated can have unexpected results, so adding .keys()
@linkmic
Copy link
Author

linkmic commented Jan 7, 2015

I made a small improvement. Still waiting on django-uuidfield to release with Python 3 fixes.

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

Successfully merging this pull request may close these issues.

2 participants