Skip to content

WIP: When searching order crates by last update date #1426

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

Closed
wants to merge 9 commits into from

Conversation

joaopapereira
Copy link

Closes #1210

crates: Vec<EncodableCrate>,
meta: Meta,
#[cfg(test)]
mod test {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this live with the rest of the integration tests, rather than re-implementing half of it here?

Copy link
Author

Choose a reason for hiding this comment

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

Are you talking about the builder or the tests?

Copy link
Contributor

Choose a reason for hiding this comment

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

Both

sort.to_string(),
current_user_id,
);
let total = crates.len() as i64;
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not correct. The query is running the equivalent of SELECT crates.*, recent_downloads.downloads, COUNT(*) OVER (). total is the number of crates that could be returned without pagination, not the number returned by the query.

}))
}

fn execute_search(
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure why this needed to be split into a separate function.

Copy link
Author

Choose a reason for hiding this comment

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

This was split to make it easier to create unit tests for this part of the code.

@carols10cents
Copy link
Member

I want to apologize for the delay in answering your questions, providing feedback, and generally moving this PR forward-- I haven't been a great maintainer lately.

Since this PR was opened, we have done quite a bit of refactoring that I think has improved the codebase, but unfortunately has made updating PRs a bit tricky. I feel bad about this too, and normally I would take on the work of resolving merge conflicts.

However, in order to try to get out from under the backlog of PRs that are outstanding, I've decided to aggressively close outdated PRs rather than update them. At this point it would probably take less time to reimplement the work rather than fix merge conflicts.

I appreciate the work you've put in here, and again I sincerely apologize for my lack of response. If you would like to work on this again in the future, I would be honored, but I also totally understand if you'd rather not.

Thank you!

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.

3 participants