-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
refactor(db): Implement query_paged_as!
macro to construct paginated queries that are still statically checked
#1600
Conversation
85535bb
to
ab4fcae
Compare
aa794f1
to
c6bb163
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1600 +/- ##
==========================================
+ Coverage 52.03% 53.90% +1.86%
==========================================
Files 113 115 +2
Lines 9263 9580 +317
==========================================
+ Hits 4820 5164 +344
+ Misses 4443 4416 -27 ☔ View full report in Codecov by Sentry. |
query_paginated_as!
macro to construct paginated queries that are still statically checked
query_paginated_as!
macro to construct paginated queries that are still statically checkedquery_paged_as!
macro to construct paginated queries that are still statically checked
crates/tabby-macros/src/lib.rs
Outdated
let skip_id = input.skip_id; | ||
let backwards = input.backwards; | ||
quote! { | ||
sqlx::query_as(&crate::make_pagination_query_with_condition({ |
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 makes tabby-macros
library only meaningful for tabby-db
, correct? Maybe move it to ee/tabby-db-macros
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.
I will do that, but I want to note that this has no actual dependency on tabby-db
and has no feature flags associated with it, since it never actually uses that code, only generates syntax which refers to it. I was figuring that it would make sense to group all our macros in one place, if we want to add more.
@@ -49,6 +49,54 @@ pub struct DbConn { | |||
cache: Arc<DbCache>, | |||
} | |||
|
|||
fn make_pagination_query_with_condition( |
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.
consider move code that will be used by macros into a separate file, e.g src/macro_deps.rs
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.
It's not only used by macros for the time being, so I think we should leave it here until the refactoring is complete.
f5be55f
to
0d648cd
Compare
Things to note:
This might be better drafted as a procedural macro? Let me know if you'd be open to that, it would be more code but wouldn't come with these compromises