-
Notifications
You must be signed in to change notification settings - Fork 645
loosen search #1560
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
loosen search #1560
Changes from 6 commits
c56e87b
810ac62
3fc3355
8f9d966
15bc5d1
5a1a85d
17c817f
41eb04b
3f19187
b69d505
346b573
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -87,6 +87,11 @@ pub const MAX_NAME_LENGTH: usize = 64; | |
type CanonCrateName<T> = self::canon_crate_name::HelperType<T>; | ||
type All = diesel::dsl::Select<crates::table, AllColumns>; | ||
type WithName<'a> = diesel::dsl::Eq<CanonCrateName<crates::name>, CanonCrateName<&'a str>>; | ||
/// The result of a loose search | ||
type LikeName<'a> = diesel::pg::expression::helper_types::ILike< | ||
CanonCrateName<crates::name>, | ||
CanonCrateName<&'a str>, | ||
>; | ||
type ByName<'a> = diesel::dsl::Filter<All, WithName<'a>>; | ||
type ByExactName<'a> = diesel::dsl::Filter<All, diesel::dsl::Eq<crates::name, &'a str>>; | ||
|
||
|
@@ -235,6 +240,21 @@ impl<'a> NewCrate<'a> { | |
} | ||
|
||
impl Crate { | ||
/// SQL filter with the `like` binary operator | ||
/// ```sql | ||
/// SELECT * | ||
/// FROM crates | ||
/// WHERE name like $1 | ||
/// ``` | ||
pub fn like_name(name: &str) -> LikeName<'_> { | ||
canon_crate_name(crates::name).ilike(canon_crate_name(name)) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I can change this back, the only reason I used There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorry about that :( |
||
} | ||
/// SQL filter with the = binary operator | ||
/// ```sql | ||
/// SELECT * | ||
/// FROM crates | ||
/// WHERE name = $1 | ||
/// ``` | ||
pub fn with_name(name: &str) -> WithName<'_> { | ||
canon_crate_name(crates::name).eq(canon_crate_name(name)) | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -414,6 +414,69 @@ fn exact_match_on_queries_with_sort() { | |
assert_eq!(json.crates[3].name, "other_sort"); | ||
} | ||
|
||
#[test] | ||
fn loose_search_order() { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This test is extremely long given how little code is being changed. Do you think this might be retesting things that are already tested elsewhere? Would it be worth trying to narrow the scope a bit to make it more clear what's being tested? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. General sort order is being addressed in other tests. I could reduce the number of creates to simplify the test. The basic idea is that we are now including crates that are partial matches. One of the previous comments about this PR was that the change should't impact current ordering so I tried to capture a few pieces of information that go into the current ordering like |
||
let (app, anon, user) = TestApp::init().with_user(); | ||
let user = user.as_model(); | ||
|
||
let ordered = app.db(|conn| { | ||
// exact match should be first | ||
let one = CrateBuilder::new("temp", user.id) | ||
.readme("readme") | ||
.description("description") | ||
.keyword("kw1") | ||
.expect_build(conn); | ||
// file shouldn't match at all | ||
CrateBuilder::new("file", user.id) | ||
.readme("readme") | ||
.description("description") | ||
.keyword("kw1") | ||
.expect_build(conn); | ||
// temp_udp should match second | ||
let two = CrateBuilder::new("temp_utp", user.id) | ||
.readme("readme") | ||
.description("description") | ||
.keyword("kw1") | ||
.expect_build(conn); | ||
// evalrs should match 3rd because of readme | ||
let three = CrateBuilder::new("evalrs", user.id) | ||
.readme( | ||
r#"$ echo 'println!("Hello World!")' | evalrs | ||
Compiling evalrs_temp v0.0.0 (file:///tmp/evalrs_temp.daiPxHtjV2VR) | ||
Finished debug [unoptimized + debuginfo] target(s) in 0.51 secs | ||
Running `target\debug\evalrs_temp.exe` | ||
Hello World!"#, | ||
) | ||
.description("description") | ||
.keyword("kw1") | ||
.expect_build(conn); | ||
// tempfile should appear 4th | ||
let four = CrateBuilder::new("tempfile", user.id) | ||
.readme("readme") | ||
.description("description") | ||
.keyword("kw1") | ||
.expect_build(conn); | ||
// mkstemp should appear 5th | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you put a bit more context in here? Why should this be 5th instead of tempfile? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The only reason that is the case is because it was inserted later. I can remove that one. |
||
let five = CrateBuilder::new("mkstemp", user.id) | ||
.readme("readme") | ||
.description("description") | ||
.keyword("kw1") | ||
.expect_build(conn); | ||
vec![one, two, three, four, five] | ||
}); | ||
let search_temp = anon.search("q=temp"); | ||
assert_eq!(search_temp.meta.total, 5); | ||
assert_eq!(search_temp.crates.len(), 5); | ||
for (lhs, rhs) in search_temp.crates.iter().zip(ordered) { | ||
assert_eq!(lhs.name, rhs.name); | ||
} | ||
let search_file = anon.search("q=file"); | ||
assert_eq!(search_file.meta.total, 2); | ||
assert_eq!(search_file.crates.len(), 2); | ||
assert_eq!(&search_file.crates[0].name, "file"); | ||
assert_eq!(&search_file.crates[1].name, "tempfile"); | ||
} | ||
|
||
#[test] | ||
fn show() { | ||
let (app, anon, user) = TestApp::init().with_user(); | ||
|
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.
Do we need the SQL examples here?
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.
They can be removed.