-
Notifications
You must be signed in to change notification settings - Fork 11
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
fix: slack-notify cli #51
Conversation
I should've done TDD 🙄 |
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 API has changed quite a bit, it's probably gonna be a bit of an effort updating it to the current state.
@@ -248,8 +248,8 @@ type channel_obj = { | |||
creator: user; | |||
is_archived: bool; | |||
is_general: bool; | |||
name_normalized: string; |
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 guess conversations.list
returns a conversation
and not a channel_obj
, so this needs to be updated somehow.
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.
It returns "a list of limited channel-like conversation objects".
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.
In such case I suggest renaming it to conversation_obj
since the Slack API calls it the "conversation type".
I think in this case the API breakage is happening anyway so might as well stick to the naming that Slack uses so it is easier to understand.
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.
Looks like channels.*
API was superseded along with im.*
and others by conversations.*
. However, there are still channel
, group
, im
, mpim
API object types, that are different from conversation
type. The codebase also has a conversation
type, which is not the type for the conversation
API object. It seems instead of attempting a fix to the notify CLI tool now, we need to overhaul the underlying types first.
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 now built on #52 which provides a new conversation_obj
type. I'm not sure about the types of id
this new API can have though.
fix: conversations.list api
channels.list
seems to becomeconversations.list
now.conversations.list
are limited conversation objects that do not havemembers
attribute.