-
Notifications
You must be signed in to change notification settings - Fork 14
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
feat: Advanced queries flag (BREAKING CHANGE) #126
Conversation
BREAKING CHANGE
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.
Looks better than #103 (thanks for the feature gate), but there are some things that still need to be done.
also it seems like the tests were not run and pushed, resulting in CI failure
let items = {table_name}.limit(page_size).offset(page * page_size).load::<Self>(db){await_keyword}?; | ||
let page = page.max(0); | ||
let page_size = page_size.max(1); | ||
let total_items = Self::filter(filter.clone()).count().get_result(db)?; |
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.
maybe in the future we can eliminate this and only do it once, so that subsequent calls dont need to also execute this (something like returning a generator / iterator)
// Table::filter() helper fn | ||
{ | ||
let diesel_backend = &config.diesel_backend; | ||
let filters = table |
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.
this array / iterator could likely be directly pushed onto the buffer, instead of collecting it in a intermediate vec (though not important to change now)
#[cfg(feature = "advanced-queries")] | ||
// generate filter struct for filter() helper function | ||
{ | ||
let filter_fields = table |
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.
same here, this could also be done without a intermediate vec (though not necessary now)
if let Some(filter_{column_name}) = filter.{column_name}.clone() {{ | ||
query = if filter_{column_name}.is_some() {{ | ||
query.filter({schema_path}{table_name}::{column_name}.eq(filter_{column_name})) | ||
}} else {{ | ||
query.filter({schema_path}{table_name}::{column_name}.is_null()) | ||
}}; | ||
}}"## | ||
) | ||
} else { | ||
format!( | ||
r##" | ||
if let Some(filter_{column_name}) = filter.{column_name}.clone() {{ | ||
query = query.filter({schema_path}{table_name}::{column_name}.eq(filter_{column_name})); | ||
}}"## |
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 really need to .clone
everything? shouldnt it be possible to just use partial-moves because the function takes in full ownership of the passed filter instead of a reference and does not return the filter itself?
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 for pointing this out -- I'll have to look into it later
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.
Co-authored-by: hasezoey <hasezoey@gmail.com>
Co-authored-by: hasezoey <hasezoey@gmail.com>
Some of this was discussed in #103 and #120