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

Family fix! #1275

Merged
merged 4 commits into from
Jun 10, 2024
Merged

Family fix! #1275

merged 4 commits into from
Jun 10, 2024

Conversation

ethteck
Copy link
Member

@ethteck ethteck commented Jun 10, 2024

No description provided.

@ethteck ethteck changed the title Family fix? Family fix! Jun 10, 2024
@ethteck ethteck merged commit 11b09b9 into main Jun 10, 2024
7 checks passed
@ethteck ethteck deleted the family branch June 10, 2024 09:46
target_assembly__hash=scratch.target_assembly.hash,
).order_by("creation_time")
else:
family = Scratch.objects.filter(slug=scratch.slug)
Copy link
Collaborator

Choose a reason for hiding this comment

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

deduplicate this and create a function that returns family? (letting this caller add an .order_by call)

target_assembly__hash=scratch.target_assembly.hash,
)
else:
family = Scratch.objects.filter(slug=scratch.slug)

return str(hash((family, request.headers.get("Accept"))))
Copy link
Collaborator

Choose a reason for hiding this comment

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

It's worth double-checking + adding a test that this actually works as it's supposed to. I wouldn't be surprised if hash() on a query set returns its id(), i.e. pretty much a random number, and that you need to call list() on it or even further serialize it for it to work.

BTW it's also not great that this query is performed twice; once for the etag and once for the content. Maybe it would be faster just to remove the etag.

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.

2 participants