Add filter() fn, update pagination() fn #103
Conversation
| .map(|column| { | ||
| let column_name = column.name.to_string(); | ||
| 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.
this looks quite redundant, is there not better option?
There was a problem hiding this comment.
feel free to suggest changes. This new filter() API is an experiment I want to run and play with. We might want to remove it in the future as intellisense gets better and better around macros.
There was a problem hiding this comment.
i had already looked, but there does not seem to be a diesel helper trait which would have a Update-like struct be the filter, something as simple as .filter(FilterStruct { someprop: Some(SomeValue), ..Default::default() }) similar to what .select(Struct::as_select()) would look like
| buffer.push_str(&format!( | ||
| r##" | ||
| #[derive(Clone)] | ||
| pub struct {struct_name}Filter {{ |
There was a problem hiding this comment.
this should probably be a new structtype
There was a problem hiding this comment.
let's leave this for now as it's an experimental API that needs more iterations before we solidify it as a core struct.
Co-authored-by: hasezoey <[email protected]>
Co-authored-by: hasezoey <[email protected]>
Co-authored-by: hasezoey <[email protected]>
| .map(|column| { | ||
| let column_name = column.name.to_string(); | ||
| 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.
i had already looked, but there does not seem to be a diesel helper trait which would have a Update-like struct be the filter, something as simple as .filter(FilterStruct { someprop: Some(SomeValue), ..Default::default() }) similar to what .select(Struct::as_select()) would look like
|
Thanks for your review @hasezoey, I've taken your feedback and applied it! 🙌 |
There was a problem hiding this comment.
I dont really like how the current filter implementation works (and i dont know a improvement), but the code otherwise looks fine, some points that could probably be changed though:
- the validate config message will look somewhat weird, because the only case it currently will be triggered is by the input being empty
- the example for the
filtermethod could make use of the table names instead of the genericTodo
it would be nice if we could somehow consistently figure out what backend is being used from the connection string, and only require it if we cant figure it out (this should be possible for sqlite, mysql and postgres, but i dont know about clap liking it)
Marking this still as "Request changes" because this PR will need to be rebased onto latest master
| @@ -441,6 +442,51 @@ impl {struct_name} {{ | |||
| }} | |||
| "##)); | |||
|
|
|||
| // Table::filter() helper fn | |||
| { | |||
There was a problem hiding this comment.
this block should likely be its own function to keep the functions smaller, but this can be delayed until we refactor this (see #105)
There was a problem hiding this comment.
Awesome, I’ll take a look at this later and try to rebase 🙌
There was a problem hiding this comment.
unless you want to delay this PR, i will hold off on merging other stuff (even if approved) so you can get this (and possibly your other PR's) merged without having to rebase it often
There was a problem hiding this comment.
Thanks @hasezoey, I really appreciate your consideration 🤗
Please do approve this PR if you think it's generally acceptable. That way, I can rebase and merge later without having to stop any others.
There was a problem hiding this comment.
Please do approve this PR if you think it's generally acceptable. That way, I can rebase and merge later without having to stop any others.
i take this as meaning you have not rebased yet, so i will merge the other standing (approved) PRs to not have to wait, after that i will approve
There was a problem hiding this comment.
Approving as per #103 (comment) to be able to rebase and merge it directly.
unless this PR will get closed because of #120
|
Also going to close this -- we can reference it later :-) |
No description provided.