faraday: Add logging formatter signatures#1037
Conversation
|
@rossta Thanks for your contribution! Please follow the instructions below for each change. Available commandsYou can use the following commands by commenting on this PR.
|
yykamei
left a comment
yykamei
left a comment
There was a problem hiding this comment.
Thanks a lot for putting this together! Adding signatures for the logging formatter is really helpful for users writing custom formatters, and the example in _test/ makes the intended usage very clear. Appreciate the contribution!
| end | ||
|
|
||
| module Logging | ||
| class Formatter |
There was a problem hiding this comment.
I think these return types should be void rather than untyped.
In the actual implementation, request, response, exception, and filter are side-effect-oriented methods: they write logs or update the formatter's filter list, and their return values are not part of the public contract.
def request: (Env env) -> void
def response: (Env env) -> void
def exception: (untyped exc) -> void
def filter: (untyped filter_word, untyped filter_replacement) -> voidThe test signature in _test/test_logging_formatter.rbs should probably use void as well, since it documents the expected override contract for custom formatters.
There was a problem hiding this comment.
Implemented in request, response, filter changes in gems/faraday/2.5. Addressed exception in gems/faraday/2.7/faraday-2.7.rbs.
| end | ||
|
|
||
| class Response | ||
| class Logger < Middleware |
There was a problem hiding this comment.
Faraday::Response::Logger also defines on_error(exc) in the implementation, but it is missing from this signature.
Could you add it here?
def on_error: (untyped exc) -> voidThere was a problem hiding this comment.
Implemented in gems/faraday/2.7/faraday-2.7.rbs
| class Logger < Middleware | ||
| def initialize: (untyped app, ?untyped logger, ?::Hash[Symbol, untyped] options) ?{ (Logging::Formatter formatter) -> void } -> void | ||
| def call: (Env env) -> untyped | ||
| def on_complete: (Env env) -> untyped |
There was a problem hiding this comment.
Since on_complete only delegates to @formatter.response(env) and the formatter method is side-effect-oriented, I think this should be void rather than untyped.
| def on_complete: (Env env) -> untyped | |
| def on_complete: (Env env) -> void |
|
|
||
| private | ||
|
|
||
| def dump_headers: (untyped headers) -> String |
There was a problem hiding this comment.
dump_headers can return nil when headers is nil in the implementation:
return if headers.nil?So this should be String? rather than String.
| def dump_headers: (untyped headers) -> String | |
| def dump_headers: (untyped headers) -> String? |
There was a problem hiding this comment.
Added signature overload in gems/faraday/2.7/faraday-2.7.rbs
| def response: (Env env) -> untyped | ||
| def filter: (untyped filter_word, untyped filter_replacement) -> ::Array[[untyped, untyped]] | ||
|
|
||
| private |
There was a problem hiding this comment.
The implementation also has a private log_errors? method, which is used by exception, but it is missing from the RBS.
Could you add it alongside log_headers? and log_body??
def log_errors?: () -> boolishThere was a problem hiding this comment.
Added to gems/faraday/2.7/faraday-2.7.rbs
rossta
force-pushed
the
feat/faraday-logging-formatter
branch
from
057a326 to
b3dbade
Compare
|
@yykamei Thank you for the thorough review! To be sure signatures were exposed in the correct VERSION/faraday.rbs, I reviewed Faraday sources for 2.5.x, 2.6.0, and 2.7.x releases. Confirmed for 2.5/faraday.rbs:
Patch availability for 2.7-only surface:
Implemented:
Verified: bin/test gems/faraday/2.5
bin/test gems/faraday/2.7Current amended commit is 2acd424. Please let me know whether this satisfies your comments. |
rossta
force-pushed
the
feat/faraday-logging-formatter
branch
from
b3dbade to
2acd424
Compare
|
/merge |
2 participants
As documented in https://lostisland.github.io/faraday/#/middleware/included/logging?id=customize-the-formatter
cc reviewer @yykamei
Thank you!