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 empty lists return null or default #1369

Merged
merged 2 commits into from Feb 24, 2017

Conversation

janwin
Copy link
Contributor

@janwin janwin commented May 20, 2016

This fix resolves the issue with integerFields trying to convert empty list in case a Nnull value is returned from the datasource

Tests are passing

@Terr
Copy link
Contributor

Terr commented May 20, 2016

Thanks for fixing this. Would you mind if I write some extra tests for this case and open a PR towards your branch?

@janwin
Copy link
Contributor Author

janwin commented May 20, 2016

Sure, go on! I would appreciate it!

@Terr
Copy link
Contributor

Terr commented May 20, 2016

I have some difficulty in reproducing the issue. Am I correct that your setup looks like this:

class LeftModel(models.Model):
    some_foreign_key = models.ForeignKey('RightModel', null=True)

class RightModel(models.Model):
    some_integer_field = models.IntegerField()

And that your model_attr is referring to some_foreign_key__some_integer_field ?

@Terr
Copy link
Contributor

Terr commented May 20, 2016

I think I got it: the ForeignKey relation/column is defined in the 'right' model? I added a test for it here: it fails without your fix and passes with it.

For some reason I don't see your fork when trying to create a PR, but if you want you can cherry pick the commit.

@janwin
Copy link
Contributor Author

janwin commented May 20, 2016

thanks @Terr i added your test to the PR

@thongly
Copy link

thongly commented Feb 24, 2017

Is this awaiting further review, or tests?

@acdha
Copy link
Contributor

acdha commented Feb 24, 2017

@thongly It looks like the CI build was just due to the multiprocessing race condition. If you wanted to rebase this against the current master branch and confirm that the tests pass that'd be helpful

@acdha
Copy link
Contributor

acdha commented Feb 24, 2017

The tests just passed after a restart

@StevenFerreira
Copy link

StevenFerreira commented Feb 24, 2017

I've just created a separate pull request (#1486) minutes ago and all tests passed locally

Edit: Just closed #1486

@acdha acdha merged commit 6006a4e into django-haystack:master Feb 24, 2017
@thongly
Copy link

thongly commented Feb 24, 2017

@acdha - Are you planning on pushing a release soon too? Wondering if we need to pip install the branch-specific version or whether we can wait on your official release. Please advise if you have a ETA.

cc: @StevenFerreira

@acdha
Copy link
Contributor

acdha commented Feb 24, 2017 via email

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

5 participants