API v2: completely rewritten with EEPs 68, 43 #12
Conversation
m-2k
force-pushed
the
master
branch
2 times, most recently
from
6753241 to
934fc55
Compare
|
@zuiderkwast do you plan to change anything in Ur repository in the future and will U consider this pull request? Thx. |
|
Hi @m-2k! It's a great update! I haven't been maintaining this project very actively. As you can see, it has very few changes over the last 10 years. If I read this PR correctly, I can just merge it and set a 2.0.0 tag, right? Nothing more is required? Then it may be good for another 10 years. |
There was a problem hiding this comment.
This is a complete rewrite of the whole library, not only the API. It is quite large and I'm not using this project anymore. I'm thinking maybe it's better that I deprecate or archive this project and link to your fork instead. What do you think?
Apart from comments below, I noticed the documentation was deleted and there is no new documentation, only examples.
| support concurrent processing of the requests in a batch, | ||
| * handles rpc calls and notifications, | ||
| * supports named and unnamed parameters, | ||
| support concurrent processing of the requests in a batch or collecting statistics; |
There was a problem hiding this comment.
I see you changed from lists:map/3 to lists:mapfoldl/3.
To process requests in parallel, you could replace lists:map/3 with a function that spawns a process for each request, waits for all of them to complete, then returns the list of replies.
How would you do that with the mapfoldl API?
Also, how would you collect statistics and get them out?
| response_error({Code, Reason, Data}) when is_integer(Code) andalso (Code < -32768 orelse Code > -32000) -> #{ | ||
| code => Code, | ||
| message => response_error_message(Reason), | ||
| data => Data | ||
| }; |
There was a problem hiding this comment.
This is not the common style in Erlang (although it's normal in languages like C with braces.)
In Erlang, the } or ], end, etc. are straight below or to the right of their opening counterpart, not to the left of it.
I'd prefer to use the OTP style, including 4 spaces indentation, like this:
| response_error({Code, Reason, Data}) when is_integer(Code) andalso (Code < -32768 orelse Code > -32000) -> #{ | |
| code => Code, | |
| message => response_error_message(Reason), | |
| data => Data | |
| }; | |
| response_error({Code, Reason, Data}) when is_integer(Code) andalso (Code < -32768 orelse Code > -32000) -> | |
| #{ | |
| code => Code, | |
| message => response_error_message(Reason), | |
| data => Data | |
| }; |
| Term -> | ||
| do_handle_term(Term, Handler, OpaqueCtxIn, MapFoldFun) | ||
| catch Class1:Reason1 -> | ||
| ?LOG_WARNING("Failed to decode request in JSON format: ~tp ~tp", [Class1, Reason1]), |
There was a problem hiding this comment.
I don't think we should log a warning here. If an untrusted user, such as remote client over the network, can send requests, they can flood the log.
If the request is malformed (syntax error, etc.) it's a client error, not a server error. It should just be reported back to the client.
| request(#{method := Mtd} = RequestSpec) -> | ||
| Method = case RequestSpec of | ||
| #{module := Mod} -> <<(atom_to_binary(Mod))/binary, $:, (atom_to_binary(Mtd))/binary>>; |
There was a problem hiding this comment.
This "module" is an undocumented extension to JSON-RPC. Why not just require the client to use "mod:func" as the method? I would prefer to avoid ad-hoc extensions and just stick to the standard.
| * `throw(method_not_found)` is reported as "Method not found" (-32601) | ||
| * `throw(invalid_params)` is reported as "Invalid params" (-32602) | ||
| * `throw(internal_error)` is reported as "Internal error" (-32603) | ||
| * `throw(server_error)` is reported as "Server error" (-32000) |
There was a problem hiding this comment.
Returning RPC errors from the handler using throw is not supported anymore? Why not?
|
I changed the About text for this repo to link to your fork instead. Let me know what you think. The next step is that I will archive this repo, if you agree. |
Yes, I’ll be able to maintain my fork and accept changes in the future. Interesting remarks. So far, the only additional work done on the repository has been adding test coverage and a consistent README.md, which is what prompted the above comments. |
2 participants
Changes:
Closes issues #5, #7, #10