feat: update sqlalchemy to use standard api instead of async one, standard has better documentation, more community support, and cleaner code #276
Conversation
…ndard has better documentation, more community support, and cleaner code
Coverage report
Click to see where and how coverage changed
This report was generated by python-coverage-comment-action |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| elif d := domain.strip(): | ||
| query = query.join(FinancialInstitutionDomainDao).filter(FinancialInstitutionDomainDao.domain == d) | ||
| return query.limit(count).offset(page * count).all() | ||
|
|
There was a problem hiding this comment.
I believe the more verbose code that was simplified here, particularly the options call, was omitted because we're already defining in the model the lazy selectin (so doing an options(joinedload...) is redundant)?
This looks much nicer
There was a problem hiding this comment.
yeah, I think the joinedload was before the lazy selectin change (kinda defeats the purpose of having lazy fetch, but all in the past now 😂); and yeap, I like the query and filter apis better
| load the async relational attributes so dto can be properly serialized | ||
| """ | ||
| for type in fi.sbl_institution_types: | ||
| await type.awaitable_attrs.sbl_type |
There was a problem hiding this comment.
what was the purpose of this? Just forcing a reload of relationship data in the model? Seemed unnecessary? Good cleanup!
There was a problem hiding this comment.
it was because the relational model was another nest deeper, so it couldn't populate the 2nd layer of nested relationship if I recall correctly, so awaitable_attrs was needed; the standard apis don't have this concern during my testing.
closes #275