-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Support users.profile.get API #298
Support users.profile.get API #298
Conversation
as always, thanks @suzuki-shunsuke |
@james-lawrence |
StatusText string `json:"status_text,omitempty"` | ||
StatusEmoji string `json:"status_emoji,omitempty"` | ||
Team string `json:"team"` | ||
Fields map[string]UserProfileCustomField `json:"fields"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a small issue with this Fields
field: Slack is inconsistent with the type of this field in the profile object.
If there are custom fields present, then fields
will be a JSON object, and all is well. If there are NO custom fields present, then fields
is an empty array (on one particularly old Slack team, fields
was actually null
?!). This causes an unmarshalling_error
for an otherwise valid profile
object.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your comment and I'm sorry for this issue.
I agree.
I send the Pull Request to revert the change.
https://github.com/nlopes/slack/pull/304
We have to think how to support custom fields.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The cleanest way would probably(?) be to have a custom unmarshaller for this struct that calls through to json.Unmarshal
for everything but Fields
(we'll have to set the struct field tag to json:"-"
), and then check the datatype of the fields
in the JSON response before unmarshalling that as well. The downside of this approach is we would have to write a custom marshaller as well.
An alternative approach is to compose some UserProfileWithCustomFields
struct with an anonymous UserProfile
field, but that would get messy, since you would have to deal with two types for essentially the same JSON blob, depending on whether or not you know there are custom fields in your Slack workspace (and GetUserProfile
functions would have to return values accordingly).
A third approach (though I'm unsure if this would actually work) would be to make the type of Fields
interface{}
so json.Unmarshal
could handle it, and then have client library code deal with making sense of it.
As I actually don't have much experience developing Golang libraries that wrap JSON APIs, I'd like to get the opinion of a project maintainer (@james-lawrence ?) as well.
It would also be nice to inform Slack of this small bug in their API, I suppose :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is one of solutions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@suzuki-shunsuke your example code looks good, minus the pointer for the field. you don't need it to be a pointer the json library handles it correctly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@james-lawrence
Thank you for your review.
I will send a pull request within tomorrow.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I send a Pull Request.
https://api.slack.com/methods/users.profile.get