-
Notifications
You must be signed in to change notification settings - Fork 96
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
Allow custom json encoding in jsonrpc handler. #165
Allow custom json encoding in jsonrpc handler. #165
Conversation
This also includes some basic tests for the jsonrpc handler.
Broken test depends on acceptance of: #164 EDIT: woops clicked wrong button. |
Not that you need my approval, but 👍 for the approach. This looks like this addresses my original beef in #133. |
quick, dirty question: why only in the rpc?
afaik, this does not solve the monkey patching problem from #133 if you think cyclone wise. |
True. We'll need more patches. Unfortunately I cannot do that atm but will be happy to review, merge, and release. |
Just to clarify here, we're proposing being able to set the json encoder and decoder at a class level for the regular request handler? How does this make sense? Perhaps for the websocket and sse handlers? |
in #133, it's discussed monkey patching on |
@vltr, I think it's probably better to separate them, and allow overriding at the RPC layer. There may be cases where one wants different behaviors in different contexts (rather than a global one, which is probably the wrong thing to do, but the easiest option I had at the time). The problem with the RPC code is that you don't have direct control over the JSON parser that is used. There may be other contexts that are similar, but I would suggest filing separate issues for each of them (as @dpnova mentioned, possibly the websocket and sse handlers). If one is doing one's own JSON parsing, one can do whatever one likes (obviously). That's just my $1.05 (NSFW). |
@posita, your $1.05 worth a laugh hahaha! well, I see no problem by this implementation being only about the json-rpc. in my handlers, I already have my own json encoder / decoder by using self.json_encode and self.json_decode. i'm not being a pita, i was just thinking about the overall. perhaps a factory or a default encoder / decoder setting would be a better idea for the rest of the api. cheers! :) |
ok, a new version of cyclone on pypi depends on this? if so, i'll propose a PR. cheers, ps: will cyclone use semantic versioning instead of |
It does make sense, we can drop the date there. |
thanks @fiorix! i'll make a PR for this json "dilemma" ;) |
…t for default jsonrpc headers for fiorix#170
Green here too @fiorix cheers! |
I'm going to accept this PR in two days. (ie Monday next week - if anyone has an issue let me know by then!) |
wow, i didn't even remember of this PR ... |
🎉 |
Stuck for too long, conflicting. |
This also includes some basic tests for the jsonrpc handler.
Note #133 for previous discussion.