Skip to content

Add transport response callback#175

Open
egnd wants to merge 1 commit into
mailru:masterfrom
egnd:add_resp_callback
Open

Add transport response callback#175
egnd wants to merge 1 commit into
mailru:masterfrom
egnd:add_resp_callback

Conversation

@egnd

@egnd egnd commented Jan 23, 2024

Copy link
Copy Markdown

Adding the ability to handle transport response

Comment thread ctx.go
type ctxKey uint8

const (
ctxRespCallbackKey ctxKey = iota + 1

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use an empty struct, as a more connonical way

Comment thread conn.go
return nil, newError(string(msg))
}

if err = callCtxRespCallback(ctx, req, resp); err != nil {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we move this callback higher so non 200 calls also get invoced?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

an error in callback should not fail the response to the database.

@DoubleDi

DoubleDi commented Feb 2, 2024

Copy link
Copy Markdown
Collaborator

Thanks for your contribution! Added small comments. Nothing Major

Comment thread ctx.go
ctxRespCallbackKey ctxKey = iota + 1
)

// RespCallback is a transport response callback.

@DoubleDi DoubleDi Feb 2, 2024

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add a comment when it is invoked and in what cases

Comment thread ctx.go
)

// RespCallback is a transport response callback.
type RespCallback func(*http.Request, *http.Response) error

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

RespCallback is short for a response callback, nothing stated about the request. Maybe we should call it TransportCallback

@juminek23-star juminek23-star left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants