-
Notifications
You must be signed in to change notification settings - Fork 169
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
Display download count on file index #563
Conversation
CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅ |
I have read the CLA Document and I hereby sign the CLA |
Cool, thanks for working on this! Let me know when it's ready for review. If you haven't already, make sure to read the contributor's guide to get your PR merged quickly. |
1583f1b
to
7fafaf9
Compare
@mtlynch this PR is ready. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool, this looks good. Just one minor note.
store/sqlite/entries.go
Outdated
LEFT OUTER JOIN | ||
( | ||
SELECT | ||
entry_id, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we use tabs here so that it's aligned with the next line?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, I fixed it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks for contributing this feature!
Fixes #494
I didn't change GetEntryMetadata, even though it was required by the description of the issue. Because download count for a single file is already implemented and used via downloads, err := s.getDB(r).GetEntryDownloads(id), len(downloads). But I can change GetEntryMetadata too if this approach (through one request to sql and left join) looks better