-
Notifications
You must be signed in to change notification settings - Fork 29
(#199) async query in table controller #236
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
Conversation
adrianhall
commented
Jan 30, 2025
- Devolved OData manipulation for better async support.
- Updated tests to support async operations.
- Disabled client-side evaluation.
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.
Copilot reviewed 37 out of 55 changed files in this pull request and generated 1 comment.
Files not reviewed (18)
- docs/public/404.html: Language not supported
- docs/public/categories/index.html: Language not supported
- docs/public/css/chroma-auto.css: Language not supported
- docs/public/css/format-print.css: Language not supported
- docs/public/css/print.css: Language not supported
- docs/public/css/swagger.css: Language not supported
- docs/public/css/theme-auto.css: Language not supported
- docs/public/css/variant.css: Language not supported
- docs/public/in-depth/client/auth/index.html: Language not supported
- docs/public/in-depth/client/index.html: Language not supported
- docs/public/in-depth/client/index.xml: Language not supported
- docs/public/in-depth/client/maui-aot-support/index.html: Language not supported
- docs/public/in-depth/client/online-operations/index.html: Language not supported
- docs/public/in-depth/index.html: Language not supported
- docs/public/in-depth/server/databases/azuresql/index.html: Language not supported
- docs/public/in-depth/server/databases/cosmos/index.html: Language not supported
- docs/public/in-depth/server/databases/in-memory/index.html: Language not supported
- docs/public/in-depth/server/databases/index.html: Language not supported
@richard-einfinity - would love your review on this - more from a "does it work with your Cosmos service implementation" |
Hi @adrianhall, yes this is more or less what I've got at the moment but with a bit more finesse :) One thing, should the OData validation settings be expanded to validate the supported OData options. With this implementation the non supported options will be ignored anyway.
When this is merged in I can drop my custom Controller and repository implementations. I'll rebase my branch and get it up onto my public repository. It's still a work in progress, I think there's about a hundred warnings to do with xml documentation to work through. I got a few more options to expand out and some error handling. But your thoughts would be much appreciated if you get the chance. Cheers Rich |
@adrianhall current cosmos work in progress here. https://github.com/richard-einfinity/Datasync/tree/Feature/Cosmos |
Hi @adrianhall, not sure if this is the right place. One thing I noticed is that maybe providing the default implementations on the interface might be counter intuitive. I.e. there's no warnings or errors if their not implemented in the class as would be with normal interface definitions. I already had CountAsync and ListAsync in my repository that I had implemented from a custom interface derived from IRepository. I removed the custom interface to inherit from IRepository directly as with this PR the methods are defined on the IRepository. My mistake I know but I didn't check the method signatures, so the functions I already had were sat there with no issues but the default implementations we're being called via the interface. Cheers Rich |
It’s the preferred mechanism for no-breaking-changes in the interface. I don’t like it either - your custom interface should have produced a warning about shadowing definitions. I would suggest that’s a problem with the compiler more than anything else (since it’s a generic problem when this happens). |