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.

Sign up for GitHub

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

Jump to bottom

Filtering out None from package lists being added to repo version #1519

Closed
wants to merge 1 commit into from
Closed

Filtering out None from package lists being added to repo version #1519

wants to merge 1 commit into from

Conversation

fao89
Copy link
Member

@fao89 fao89 commented Nov 18, 2019

@fao89 fao89 mentioned this pull request Nov 18, 2019
Closed
@daviddavis
Copy link
Contributor

Ah good catch. added_modules is defined in two places so I think it might be best to make this change to line 36 instead.

@fao89
Copy link
Member Author

fao89 commented Nov 18, 2019

Ah good catch. added_modules is defined in two places so I think it might be best to make this change to line 36 instead.

performance tests are great, they caught it

@fao89 fao89 changed the title Assuring pk is present when add repository content WIP - Assuring pk is present when add repository content Nov 18, 2019
@ipanova
Copy link
Member

ipanova commented Nov 19, 2019

Ah good catch. added_modules is defined in two places so I think it might be best to make this change to line 36 instead.

+1

@fao89 fao89 changed the title WIP - Assuring pk is present when add repository content Assuring pk is present when add repository content Nov 19, 2019
daviddavis
daviddavis reviewed Nov 19, 2019
View reviewed changes
CHANGES/5746.bugfix Outdated
@@ -0,0 +1 @@
Assuring pk is present when adding repository content
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you update this and the commit message?

@fao89 fao89 changed the title Assuring pk is present when add repository content Filtering out None from package lists being added to repo version Nov 19, 2019
daviddavis
daviddavis previously approved these changes Nov 19, 2019
View reviewed changes
@daviddavis daviddavis dismissed their stale review November 19, 2019 19:12

I need to remove values_list

@daviddavis
Copy link
Contributor

I read through the django docs for values_list() and it looks like it won't work reliably for many-to-many associations. My testing confirms it. I'm happy to take over this PR?

@daviddavis
Copy link
Contributor

Closing in favor of #1524

@daviddavis daviddavis closed this Nov 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@daviddavis daviddavis daviddavis left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@fao89 @daviddavis @ipanova