Skip to content
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

Add client callback for API warning messages #913

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

ghost
Copy link

@ghost ghost commented Mar 19, 2021

Dupe of #905, except that it doesn't contain all the changes to our forked master branch anymore.


Problem: There's no way to surface Slack's API warnings in application code from the library.

This PR exposes a new client Option that registers a callback for every response that comes back with warning information. This suits the use case for logging/tracking warnings as they appear, without affecting the request/response interface of each endpoint. The nodejs sdk seems to take a similar approach.

onWarning := func(warning *slack.Warning, path string, values url.Values) {
  // Log the warning here.
}
client := slack.New(token, slack.OptionOnWarning(onWarning))

An easy way to test this and see warnings is to update outgoing headers to have ; charset=utf-8 at the end.

Relevant docs/reading:

ryannjohnson-range added 2 commits February 26, 2021 03:12
The Slack web api returns a "warning" property on response objects. The
requests succeed, but this field will return a comma-separated list of
warning codes that range from "superfluous_charset" to
"method_deprecated", as well as extra messages in the
.response_metadata.warnings property.

This option allows applications to get notified when these warnings
exist.
@kanata2
Copy link
Member

kanata2 commented Nov 6, 2021

@ryannjohnson-range
Since #939 was recently merged, it is now possible for users to retrieve warnings.
If there is no special usecase where we want to execute the callback,
I think this PR should be closed, what do you think?

@kanata2 kanata2 added the feedback given When a review has been conducted and awaiting the response from the comitter(s) label Nov 6, 2021
@ghost
Copy link
Author

ghost commented Nov 8, 2021

Warnings still look inaccessible if the response succeeds.

The use case I'm concerned about for warnings is logging, specifically to alert me when an API I'm using is being scheduled for deprecation. It makes sense to me to have this occur at the client-level since I don't expect my application logic to fork based on warnings.

@kanata2
Copy link
Member

kanata2 commented Nov 8, 2021

Warnings still look inaccessible if the response succeeds.

Ah,I missed this case. Thanks!

@kanata2 kanata2 removed the feedback given When a review has been conducted and awaiting the response from the comitter(s) label Nov 8, 2021
@zchee zchee requested review from kanata2 and zchee February 26, 2022 10:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants