Skip to content

Show and sort by downloads on reverse dependency page #531

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

Merged
merged 7 commits into from
Feb 20, 2017
Merged

Show and sort by downloads on reverse dependency page #531

merged 7 commits into from
Feb 20, 2017

Conversation

LeopoldArkham
Copy link
Contributor

@LeopoldArkham LeopoldArkham commented Jan 29, 2017

Fixes #495

I swapped the redundant arrow link on the right for the download count, I don't think anybody used it (?).

@LeopoldArkham
Copy link
Contributor Author

@carols10cents Ok, it works now. I had to abuse the CI because the tests won't run on my machine.
Let me know how it looks.

@carols10cents
Copy link
Member

@LeopoldArkham Hm, why doesn't the ordering in SQL work?

@LeopoldArkham
Copy link
Contributor Author

LeopoldArkham commented Feb 3, 2017

@carols10cents Whenever a query contains a DISTINCT ON statement and an ORDER BY clause, the leftmost argument to ORDER BY has to be the same as the one to DISTINCT.

So since we're DISTINCT ON crate_name, I can only ORDER BY crate_name. I think it would be possible to do in all SQL with a sub-query, but this seemed like a clearer and more maintainable way.

If there's a policy against this (or I'm missing something), I can go back and look into it.

(Didn't mean to close)

@carols10cents
Copy link
Member

Oh! There's no policy or anything, it's just that SQL is really good at ordering things really fast :)

So the reason a reverse dependency might be duplicated is if a crate lists another crate as a dependency multiple times, such as target-specific dependencies. So for crate A, it might depend on crate B twice, so crate B's reverse dependencies would have duplicate listing of A. However, in both listings of A, the number of crates.downloads for A will be the same. So we should be able to add crate_downloads to the list of columns specified in the DISTINCT clause, and meet the necessary criteria for the ordering, like this:

SELECT DISTINCT ON (crate_downloads, crate_name)
    dependencies.*,
    crates.downloads AS crate_downloads,
    crates.name AS crate_name
FROM dependencies
INNER JOIN versions
ON versions.id = dependencies.version_id
INNER JOIN crates
ON crates.id = versions.crate_id
WHERE dependencies.crate_id = 2
AND versions.num = crates.max_version
ORDER BY crate_downloads DESC;

What do you think? Is there anything I'm missing? (it's entirely possible!)

@LeopoldArkham
Copy link
Contributor Author

No, you're totally right, thank you! I had misunderstood the behavior of DISTINCT ON when provided with more than 1 argument. (See? No programming Sunday evenings.)

I also got the test suite to (mostly) work, so we should be OK.

@LeopoldArkham
Copy link
Contributor Author

^ I jinxed myself.
Although it's the yarn install that failed. Gonna close and re-open to trigger a rebuild.

@carols10cents
Copy link
Member

Looks great, works like a charm!!! 🍀 Sorry I took so long to review this, thank you for your patience!! ❤️

@carols10cents carols10cents merged commit a8be938 into rust-lang:master Feb 20, 2017
@LeopoldArkham
Copy link
Contributor Author

And you for yours!

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