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

Fix SearchFilter to-many behavior/performance #5264

Merged
merged 2 commits into from Jul 11, 2017

Conversation

rpkilby
Copy link
Member

@rpkilby rpkilby commented Jul 10, 2017

Fixes #4655. Thanks @cdosborn for the patch!

@rpkilby rpkilby force-pushed the search-filter-reverse branch from 2f0e932 to d1cfec8 Compare Jul 10, 2017
@jpadilla
Copy link
Member

jpadilla commented Jul 11, 2017

@rpkilby thanks for progressing this and getting it done!

@jpadilla jpadilla merged commit 8a8389b into encode:master Jul 11, 2017
1 check passed
@jpadilla jpadilla added this to the 3.6.4 Release milestone Jul 11, 2017
@jpadilla jpadilla added the Bug label Jul 11, 2017
@rpkilby rpkilby deleted the search-filter-reverse branch Jul 11, 2017
@tomchristie
Copy link
Member

tomchristie commented Jul 12, 2017

Great stuff @rpkilby, and thanks for reviewing @jpadilla!

@rpkilby
Copy link
Member Author

rpkilby commented Dec 16, 2019

Hi @Pomax. The best thing to do is to create a failing test case as a PR. You don't have to fix the bug yourself, but a failing test case gives us something to work with.

Pomax added a commit to mozilla/network-pulse-api that referenced this pull request Dec 16, 2019
@Pomax
Copy link

Pomax commented Dec 16, 2019

@rpkilby I've filed #7094, but ran into a bit of an issue in that I can't seem to run the tests for the "between 3.6.3 and 3.6.4" version that this PR lives in. I'll be more than happy to further discuss that in the testcase PR.

@rpkilby
Copy link
Member Author

rpkilby commented Dec 16, 2019

I can't seem to run the tests for the "between 3.6.3 and 3.6.4" version that this PR lives in

That's fine. We just need a failing test case against the current master. If you can run the tests locally against v3.6.3 and verify that it succeeds, then that's sufficient.

@Pomax
Copy link

Pomax commented Dec 17, 2019

3.6.3 also seems to not want to get to a point where tests will run... are there additional steps beyond what's in the old CONTRIBUTE.md (e.g. which python version to use for the virtualenv, which django version to peg, etc?) to make the tests run successfully?

(I tried both macos and windows, with python 3.7 and python 2.7 just to see if that was the problem -- it wasn't, django-guardian needs 3.5 or above =D -- with django pegged to 1.11 because this is code from 2017, but nothing seems to get me to a point where the tests will run, unlike running the tests for current master, which worked instantly)

@rpkilby
Copy link
Member Author

rpkilby commented Dec 17, 2019

I'm also trying to get this to work on ~3.6.3, but I usually try to run tests via tox. In order to get this to work, I had to unpin the dependencies in requirements-testing.txt, then I could run:

tox -e py37-django111

Edit:
I'm going to continue this discussion in the PR.

@Pomax
Copy link

Pomax commented Dec 17, 2019

ah, nice. Let me see if I can make that work.

@Pomax
Copy link

Pomax commented Dec 17, 2019

Looks like it's still not quite working (windows gives 58 errors, macos 1) but it's close enough to probably finish up tomorrow morning (Vancouver Island time). Thank you for the help!

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

Successfully merging this pull request may close these issues.

4 participants