feat: add interface stubs for async adapters#335
Conversation
|
@Nekotoxin please review |
|
@thearchitector fix: |
|
@thearchitector breaking changes are not allowed. Python is a dynamic language. I believe there must be ways to do this without any breaking changes (like via dynamic casting). So closed for now |
|
@hsluoyz the function definitions themselves are different between the example implementation (async file adapter) and the rest. that is not what Python supports when you say "dynamic"; so at the very least with no other changes, that impl change will be required --- technically the only "non-breaking" kinda feasible way given your constraints is to have the async implementations be synchronous functions that return coroutines vs already being coroutines. in the greater scope, though, I think the position is somewhat foolish; no breaking changes means there is an implicit trust that the original and initial implementation was the best way to do it -- having looked through the code, I can guarantee that is not true (nor is it ever true with anything).... So what you're actually saying is "foundational improvements are not allowed," and that is the textbook definition of tech debt. if that's the position and sacrifice, that's fine - nothing I can do about it anyways - but it should be made explicitly clear in the docs and CLA. |
|
it is worth mentioning, I suppose, that doing the "sync returns coro" approach will work, but will also mean any existing adapters that use of course, the problem can also just be ignored completely since it's not a runtime bug, in which case no PR is necessary. that's always an option |
|
@thearchitector Hi,
What I don't support are:
I look forward to your revision and I will reopen this PR. |
|
@thearchitector fix: |
@leeqvip @hsluoyz thanks for the feedback. to be clear, you support everything except moving the interfaces, correct? So all the existing interfaces ( |
|
@thearchitector yes |
|
@hsluoyz @leeqvip i've moved the interfaces back to also, running |
leeqvip
left a comment
There was a problem hiding this comment.
Thanks.
In the same way, please move update_adapter to the persist directory.
Regarding the linter warning you mentioned, I didn’t see it?
https://github.com/casbin/pycasbin/actions/runs/7312454795/job/19923199403#step:4:1
@leeqvip it doesn't happen in the github action because super linter pins the broken dependency to an old version, but shows up locally because the dependency isn't pinned in we can solve it by pinning the hidden dep in the local req.txt file to an older version, but i think it would be best to just bump the super linter version to |
So, bump to the new version plz. |
|
@thearchitector fix CI errors first: |
|
@hsluoyz huh, gh actions cannot find the super linter version. weird, it definitely exists... do I need to address the coveralls stuff too? Not sure what the error is there (the test percentage regression?) |
# [1.34.0](v1.33.0...v1.34.0) (2023-12-28) ### Features * add interface stubs for async adapters ([#335](#335)) ([d557189](d557189))
|
🎉 This PR is included in version 1.34.0 🎉 The release is available on:
Your semantic-release bot 📦🚀 |
* refactor!: change reorganize imports for sane asyncio ext * feat: async adapter interface stubs * chore: reflect pr comments * chore: revert black bump pending decision * refactor: move update adapter interface to persist module * ci: bump tooling and linting versions * ci: fix linting action
# [1.34.0](apache/casbin-pycasbin@v1.33.0...v1.34.0) (2023-12-28) ### Features * add interface stubs for async adapters ([apache#335](apache#335)) ([d557189](apache@d557189))



Closes #334.
This PR adds asyncio stubs for all the adapter interfaces in order to draw a strict line between asyncio-based adapters (where all functions are expected to be coroutines) and the sync ones (where they are not).
To make things make more sense, I've renamed the existing
casbin.persist.adapters.adapter_filtered::FilteredAdaptertocasbin.persist.adapters.filtered_file_adapter::FilteredFileAdapter, since the latter is more accurate and better organizes the module structure.All of the new interfaces + the existing
async_file_adapterare now withincasbin.persist.adapters.asyncioas to partition them better; i've tried retaining most of the imports as to avoid too many breaking changes.but, there are 3 breaking changes:
AsyncEnforcernow requires the adapter instance you pass to be an instance ofAsyncAdapternotAdapter.adapters.FilteredAdapteris nowadapters.FilteredFileAdapter---> the newadapters.FilteredAdapteris now the oldpersist.FilteredAdapterinterface.from casbin.persist.update_adapter import UpdateAdaptervs.from casbin.persist import UpdateAdapter) will have to change to the new imports.