-
Notifications
You must be signed in to change notification settings - Fork 120
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
authentication #594
base: trunk
Are you sure you want to change the base?
authentication #594
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## trunk #594 +/- ##
==========================================
+ Coverage 99.07% 99.33% +0.26%
==========================================
Files 46 71 +25
Lines 3986 5591 +1605
Branches 531 770 +239
==========================================
+ Hits 3949 5554 +1605
- Misses 23 26 +3
+ Partials 14 11 -3 ☔ View full report in Codecov by Sentry. |
- move memory session store to its own dedicated package - implement MemoryAccountStore (i.e. ISimpleAccount/ISimpleAccountBinding authorizers)
at least on pypy3.9, typing.Protocol appears to runtime-check its args to make sure they are all TypeVars (i.e. not ParamSpecs)
There's a lot here, but I think now that there is documentation it's possible to get a holistic view of the whole shebang. |
Hi @glyph I don't know what to say / do about this PR. I have never used klein and I don't have experience in terms of API design for micro web frameworks. Also, for templating I am just using Jinja2... and I am perfectly fine with IResource.putChild This is one reason why I think that is best for klein to be a separate repo/project, and not merged into twisted/twisted. but this PR is in review for a long time, and it looks like nobody else cared about this API :( So if you are using this, I think is best to merge this as it is, at least you don't have to use a fork of klein. at a quick look, the PR looks good. So as long as the tests pass, I think is best to merge it. thanks |
Yeah, I think Klein has a sufficiently distinct idiom that it should remain separate. That said some things probably ought to be merged back… |
Thanks! Right now my plan is to put the large-new-library bit of this — https://github.com/glyph/dbxs — into separate package so it can have its own documentation and not have a big digression in the middle of the Klein docs about a whole new database thing just so we can avoid a big sqlalchemy dependency. But once I've released that, I'll land this on the strength of this review. |
Looking forward to dbxs. For my project, which only simple sqlite access, I ended up with a custom layer to prevent sqlinjection, construct simple select queries, and handle migration between schema changes, and wrapping the thread based API for sqlite to a deferred API. have you checked peewee ? |
As it looks like others don't care that much about klein, I think that as long as all tests/checks pass on this PR, it can be merged. Otherwise, I don't know when it will be reviewed. If this is merged, maybe people will start using it and send feedback later. Cheers |
It's not async, so it's a bit of a nonstarter :) |
I now see that the author of But there is this project https://github.com/klen/peewee-aio going back to this PR. I think that this should be merged without a review. It looks like there is no team to maintain klein ... so PR can be merged by the curent single maintainer witour a review ... (as there is noone else to review the PRs) |
The sessions layer is missing some stuff. This is an omnibus PR that adds it, and will add docs for it.
I would recommend starting the review by looking through the new documentation at https://klein--594.org.readthedocs.build/en/594/ — specifically https://klein--594.org.readthedocs.build/en/594/introduction/3-forms.html and https://klein--594.org.readthedocs.build/en/594/introduction/4-auth.html .
It's probably too big to land as-is. In fact, some parts of this are probably entirely new, separate projects that should be a dependency.Some of this stuff might be worth spinning out, but looking at the documentation, it would be difficult to land it piecemeal and get a cohesive story about what it's all for.