-
Notifications
You must be signed in to change notification settings - Fork 85
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
V2 of asyncjsonwrap #373
V2 of asyncjsonwrap #373
Conversation
@H-Shay Looks like this has some conflicts due to merging the other branch! |
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.
Looks great overall, since we're not using callbacks anymore we can clean the code up a bit!
Signed-off-by: H.Shay <[email protected]>
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.
Looks good with the minor change!
sydent/http/servlets/__init__.py
Outdated
|
||
:param f: The original function passed to the asyncjsonwrap decorator |
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.
f
isn't a parameter to inner
, so I think we should leave it out from here!
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.
Right it's a param to render. I wasn't paying attention there!
Here is the second version of
asyncjsonwrap
, based on the code skeleton you gave me. I did not add a changelog to this one because I figured only one of these PRs would be merged, so I'd add the proper changelog file to whichever version is the final one. The jury is out on which version is more readable/cleaner...