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

Allow filtering versions by active #4414

Merged
merged 2 commits into from Aug 1, 2018

Conversation

Projects
None yet
3 participants
@davidfischer
Contributor

davidfischer commented Jul 23, 2018

This allows making an API requests like:

I believe performance should be ok given this is just a boolean field.

@davidfischer davidfischer requested a review from rtfd/core Jul 23, 2018

@humitos

Looks good!

Shouldn't we have a simple test case for these changes? At least, only those that we are sure that are needed for some projects to work properly so we don't break them in the future.

I think we never had tests and when we removed django-filters we didn't realize that we broke some user's projects.

@davidfischer

This comment has been minimized.

Show comment
Hide comment
@davidfischer

davidfischer Jul 23, 2018

Contributor

I'm not opposed to a test but it is testing functionality in django-filter and not really in our project. This test would not actually test code we've written but verify that things are setup appropriately.

Contributor

davidfischer commented Jul 23, 2018

I'm not opposed to a test but it is testing functionality in django-filter and not really in our project. This test would not actually test code we've written but verify that things are setup appropriately.

@humitos

This comment has been minimized.

Show comment
Hide comment
@humitos

humitos Jul 23, 2018

Member

I agree with that. I'm fine on having some very simple/basic test that checks the setup, though. What do you think?

Member

humitos commented Jul 23, 2018

I agree with that. I'm fine on having some very simple/basic test that checks the setup, though. What do you think?

@davidfischer

This comment has been minimized.

Show comment
Hide comment
@davidfischer

davidfischer Jul 23, 2018

Contributor

Works for me.

Contributor

davidfischer commented Jul 23, 2018

Works for me.

@humitos

Thanks!

self.assertEqual(resp.status_code, 200)
self.assertEqual(resp.data['count'], pip.versions.filter(active=True).count())
# Do the same thing for inactive versions

This comment has been minimized.

@humitos

humitos Jul 24, 2018

Member

nit: this could be achieved in a fancy way by using pytest parametrize: https://docs.pytest.org/en/latest/parametrize.html

@humitos

humitos Jul 24, 2018

Member

nit: this could be achieved in a fancy way by using pytest parametrize: https://docs.pytest.org/en/latest/parametrize.html

This comment has been minimized.

@davidfischer

davidfischer Jul 24, 2018

Contributor

I actually think that would make the test less clear.

@davidfischer

davidfischer Jul 24, 2018

Contributor

I actually think that would make the test less clear.

This comment has been minimized.

@davidfischer davidfischer merged commit 9af5d56 into master Aug 1, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@davidfischer davidfischer deleted the davidfischer/apiv2-active-versions branch Aug 1, 2018

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