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

Allow filtering versions by active #4414

Merged
merged 2 commits into from Aug 1, 2018

Conversation

@davidfischer
Copy link
Contributor

@davidfischer 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 Jul 23, 2018
Copy link
Member

@humitos humitos left a comment

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
Copy link
Contributor Author

@davidfischer 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
Copy link
Member

@humitos 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
Copy link
Contributor Author

@davidfischer davidfischer commented Jul 23, 2018

Works for me.

Copy link
Member

@humitos humitos left a comment

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
Copy link
Member

@humitos humitos Jul 24, 2018

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

Copy link
Contributor Author

@davidfischer davidfischer Jul 24, 2018

I actually think that would make the test less clear.

Copy link
Member

@stsewd stsewd Jul 24, 2018

@davidfischer davidfischer merged commit 9af5d56 into master Aug 1, 2018
1 check passed
@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
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants